-
Notifications
You must be signed in to change notification settings - Fork 32
feat(Anchor + Button): allow both leading and trailing icons #3141
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
base: main
Are you sure you want to change the base?
Conversation
View your CI Pipeline Execution ↗ for commit 7e37ce1 ☁️ Nx Cloud last updated this comment at |
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.
code looks good! have a couple of comments around icon alignment + some story rendering
export const InlineIcons: Story = { | ||
render: () => ( | ||
<FlexBox gap={16} row> | ||
<FlexBox flexDirection={{ _: 'column', sm: 'row' }} gap={16}> |
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 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.
something weird i've noticed is that disabled links no longer apply disabled styles and don't change over to buttons. this wasn't introduced in this PR but i'm curious when it started to happen
iconPosition: 'right', | ||
iconAndTextGap: 16, | ||
}); |
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.
nit: i'm sortof anti explicit style testing, but i think we can just think about cleaning these up when we eventually introduce visual testing
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.
Alrighties, I forgot about this tbh and some of the tests were AI-gen tbh
I did remove the ones related to spacing
but I kept the layout related ones, thoughts?
📬Published Alpha Packages:@codecademy/[email protected] |
🚀 Styleguide deploy preview ready! |
Overview
Updates the Anchor and Button components to allow both leading and trailing icons
PR Checklist
Testing Instructions
Don't make me tap the sign.
?path=/docs/typography-anchor--docs&globals=viewport:responsive
and scroll down to the "Icons" section?path=/docs/atoms-buttons-button--docs
and scroll to the "Inline icons" section/?path=/docs/molecules-menu--docs
PR Links and Envs