-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Shadcn migration - language picker #13531
Conversation
…fect for filtering
✅ Deploy Preview for ethereumorg ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Does that make any negative impact to the UX? IMO:
Agree. However, I tested it on a physical phone and didn't get the focus nor the keyboard popping up. Would be nice to have a second test to confirm that we don't see the keyboard. |
do you mean this PR (focus on the input) would only affect desktop and mobile? |
As far as I have tested it, the focus only happens on desktop. On mobile devices, I have to manually click on the input to filter things. |
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 looks great @pettinarip! Can we just update status of TODO's and clear any conflicts? Think we can pull in from there
// TODO double check this | ||
// sx={{ | ||
// p: { | ||
// textDecoration: "none", | ||
// overflow: "hidden", | ||
// textOverflow: "ellipsis", | ||
// whiteSpace: "nowrap", | ||
// }, | ||
// }} |
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.
Haven't encountered any need for this while testing it out.
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.
Thank, yes, I was trying to see where this was coming from but I think we could clean this up since it doesn't seem to add anything atm.
Done. I think we are ready for another review. |
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 awesome! Great job @pettinarip! Pulling in
@@ -0,0 +1,171 @@ | |||
import * as React from "react" | |||
import { Command as CommandPrimitive } from "cmdk" |
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.
Discovering these new packages along the way with the shadcn-ui approach. Loving it.
DEPENDS ON:
Description
Language picker implementation + refactor using Shadcn's Popover, Command, Progress, and Badge components.
TODO
ellipsis
styles on the component item