-
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
chore(AppBar): move appbar from brand #3038
base: main
Are you sure you want to change the base?
Conversation
View your CI Pipeline Execution ↗ for commit 23d79f4. ☁️ Nx Cloud last updated this comment at |
📬Published Alpha Packages:@codecademy/[email protected] |
🚀 Styleguide deploy preview ready! |
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.
There's some leftover brand stuff that I think can be touched up
alignItems="center" | ||
height="100%" | ||
size={wide ? 'wide' : 'medium'} | ||
zIndex={14} |
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.
This feels like an arbitrary value lol
should it always be 14? should/could this value potentially be set by a prop?
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.
yeha that makes sense, i'll make it a prop
<Canvas> | ||
<Story of={AppBarStories.Header} /> | ||
</Canvas> |
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.
For some reason this shows up in the page as the default story —
You can change it: <Canvas of={AppBarStories.Header} />
to get it show properly
It'd also be good to include a playground section below this and maybe format the whole story closer to the other stories we use for gamut's story pages
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 i just did a straight copy-paste, i'll fix it more
Overview
Moves AppBar from Brand to Gamut - first in a project to decouple AppHeader components from business logic
PR Checklist
Testing Instructions
PR Links and Envs