-
Notifications
You must be signed in to change notification settings - Fork 28
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
Enhance search bar UX #83
Conversation
Looks good to me! |
-moz-transform: scale(-1, 1); | ||
-webkit-transform: scale(-1, 1); | ||
-o-transform: scale(-1, 1); | ||
-ms-transform: scale(-1, 1); |
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.
Which browsers need these prefixes?
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.
https://caniuse.com/?search=transform
There are about 0.91% of users in total globally still using older browsers needing these prefixes, I used to just add these prefixes, but maybe it's not a good practice, should I just remove them?
(Just found that -o-
is used for Opera 11.5 and no one is using that version anymore, I'm going to remove it first)
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.
Generally speaking we've followed the @wordpress/browserslist-config
for which browsers to support.
Browser List
and_chr 101
android 101
chrome 101
chrome 100
chrome 99
edge 101
edge 100
firefox 100
firefox 99
ios_saf 15.4
ios_saf 15.2-15.3
ios_saf 14.5-14.8
op_mini all
opera 86
opera 85
safari 15.5
safari 15.4
samsung 16.0
The only one on this list that doesn't support unprefixed transform is Opera Mini, but it doesn't support transform at all, so we should be okay to drop the prefixes. In the past we've had people flag issues in older browsers (WordPress/wporg-mu-plugins#159), so I'd also check the support for Chrome 87 and make sure it's not broken, at least. But we're fine with unprefixed transform here, too 👍🏻
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.
I found a few issues:
f7ec437
to
7b95af1
Compare
Thanks for catching the issue on Firefox and on smaller screens, I've fixed them in the commit: Fix incorrect style, could you help review them again, thanks! Tested on Firefox and chrome as well as on different screen sizes: https://d.pr/i/rb0Nt2 |
7b95af1
to
837e8c1
Compare
border-left: unset; | ||
margin: 0; | ||
|
||
&:active { |
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.
&:active { | |
&:focus { |
The keyboard focus state is controlled with :focus
. You can test this by using your keyboard, click into the search bar, then hit tab. It will move you to the button, but there's no visual indicator in the PR currently.
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.
|
||
&:active { | ||
outline-style: auto; | ||
outline-color: rgb(0, 95, 204); |
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.
Where does this color come from? Maybe instead, try one of the colors in this list to keep the palette consistent. Like this:
outline-color: rgb(0, 95, 204); | |
outline-color: get-color(blue-60); |
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.
I got it from Chrome devTools directly.
the colors in this list to keep the palette consistent
I like the idea, thanks for sharing it! 👍 However, after applying it, I found the color is a bit different in Chrome (it's hard to tell without zooming in or looking carefully into it though), and it's somehow the same in Firefox. I think I'll change it to use get-color
here anyway as it seems to me it's just a slight difference.
Done Changing.
32b487c
to
6a18eb1
Compare
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 great now! Thanks for iterating on it :)
6a18eb1
to
30d5117
Compare
30d5117
to
7685b63
Compare
This PR makes three simple changes to the search bar UX mentioned here.
Search ...
toSearch in Code Reference ...
.As to how to make the search button in the search bar more obvious, since we might be able to just use the search on top of the handbook sidebar (comment) or just move the search bar control into the site-title (PR), I think it isn't worth spending time for the moment to figure out whether to implement any of the proposals or just remove the whole search bar.
Screenshots
How to test