-
Notifications
You must be signed in to change notification settings - Fork 1k
add option in anchor for gap per size #7491
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
Conversation
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.
LGTM
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.
Looks good!
@@ -272,6 +285,11 @@ describe('Anchor', () => { | |||
</Paragraph> | |||
<Anchor label="Default anchor with no size prop should receive medium" /> | |||
<Anchor label="Anchor with icon" icon={<LinkNext />} /> | |||
<Anchor | |||
label="Large anchor with icon should receive color on icon" |
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.
Should update the anchor name since it isn't a large anchor
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.
Looks good!
What does this PR do?
This PR adds
anchor.size.gap
define a gap per anchor size.Where should the reviewer start?
Anchor.js
What testing has been done on this PR?
locally and test
How should this be manually tested?
locally
Do Jest tests follow these best practices?
screen
is used for querying.asFragment()
is used for snapshot testing.Any background context you want to provide?
Right now, the gap between anchor label + icon can only be defined once (theme.anchor.gap) but it would be useful to be able to define a gap per anchor size.
What are the relevant issues?
closes #7477
Screenshots (if appropriate)
Do the grommet docs need to be updated?
yes
Should this PR be mentioned in the release notes?
yes
Is this change backwards compatible or is it a breaking change?
backwards compatible