-
Notifications
You must be signed in to change notification settings - Fork 5k
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: make Filters clickable on mobile to open filters menu #12541
Conversation
✅ Deploy Preview for ethereumorg ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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! Just left one question that I don't see as a blocker, but wanted to raise... can address separately.
<Text | ||
hideFrom="lg" | ||
lineHeight={1.6} | ||
fontSize="md" | ||
color="primary.base" | ||
textTransform="uppercase" | ||
cursor="pointer" | ||
onClick={onOpen} | ||
> |
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.
Just curious, is there any reason we don't want to use a button here? Seems more appropriate from an a11y standpoint
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.
Agreed. Now, we are basically turning this component into a button.
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.
@wackerow It was defined in the design that should behave like a button but without the button styles. I think I can add the as={button}
prop to improve a11y
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.
Okay cool.. yeah that raising a small design system question... we have variant="link"
for our buttons, but this will bold and underline the text (inconsistent with what we want here)... @nloureiro Should we have another variant that is even more simplified for buttons like this? Or should we convert this one to use the link
variant styling?
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.
@wackerow do you think that could be a blocker for this PR or could be address later? I've added the as="button"
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.
Nope, not at all! We can address design system stuff separately, this works
This PR makes the "Filters" label clickable to allow opening filters menu on mobile