-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(Flyout): Add as prop for Flyout title #2925
Conversation
📬Published Alpha Packages:@codecademy/[email protected] |
🚀 Styleguide deploy preview ready! |
@@ -72,7 +72,7 @@ export const Flyout: React.FC<FlyoutProps> = ({ | |||
maxWidth="100%" | |||
justifyContent="space-between" | |||
> | |||
<Text as="h2" fontSize={22}> | |||
<Text as="h1" fontSize={22}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you think it makes sense for this to be configurable and default to h1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm i had that initially but i couldn't think of a case when title
wont be the first in the heading level
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that makes sense - we can swap to h1 and just refactor if there are any cases of it needing another title level
Overview
as
prop for Flyout title heading-levelPR Checklist
Testing Instructions
PR Links and Envs