-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Fix: Consistent UI #23202
Fix: Consistent UI #23202
Conversation
Size Change: -195 B (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
font-weight: 400; | ||
color: $dark-gray-900; | ||
font-weight: 500; | ||
color: $dark-gray-primary; |
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.
Can't we remove the color and rely on the inheritance instead or is this really needed?
position: relative; | ||
z-index: z-index(".edit-post-sidebar__panel-tab.is-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.
Why do we need a z-index?
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.
@@ -17,7 +17,7 @@ import { | |||
} from '@wordpress/components'; | |||
import { withSelect, withDispatch } from '@wordpress/data'; | |||
import { compose } from '@wordpress/compose'; | |||
import { close } from '@wordpress/icons'; | |||
import { closeSmall } from '@wordpress/icons'; |
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 we replace all close icons or do you think there's a need for a small and a big one?
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 think there's a use case for both, honestly, it's okay they're big in the modal dialogs, but in the sidebar contexts they should not be quite as notable.
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.
While I didn't test deeply, by looking at the code, the changes seem safe for me.
Also, let's try to not add new changes and use separate PRs to make things easy to test. Ideally, each change happens in its own PR. |
in my testing, the toggle with I/O feels a bit too wide (especially when it's unchecked), do you think we can reduce its width? |
Its border width also feels inconsistent with the inputs... a 1px border might look better IMO. |
This is also the same for checkbox so we might want to do it separately if ever. |
I updated the margin to match that of the checkbox.
I think I agree, but I'd like to polish both the toggle and checkbox and radio separately, they're all 2px now. |
I actually was thinking about the toggle width itself :) Regardless this PR looks good. |
Ah. Yep, I've seen a G2 design that I want to look at. Thanks. |
7c7cf7a
to
504af49
Compare
I see failing e2e tests that block this PR. It’s a recurring issue that visual changes break them. We need to rethink approach how user interactions are coded, my guess is that tests fail when mouse cursor is moved. @youknowriad, could we limit mouse interactions in e2e tests to absolute minimum and use keyboard interactions instead? |
Thanks so much for looking, Gzregorz. I'm banging my head against the tests on this one, I'd love to learn how to fix them, I've definitely fixed a few in the past. But these confuse me to no end. It's the same, frankly, with #22213, which is a great code quality improvement that would be nice to land also. |
504af49
to
71ec017
Compare
Tests are passing! Turns out there are two ways to update snapshots and I was only familiar with the one: |
This PR does a number of things in the name of addressing or almost addressing the remaining items in #18667. In additionally, it fixes all items from #19904. Here's what it does:
It makes the buttongroup consistent with regards to toggles:
Note to third parties, the inactive buttons aren't secondary, they're just buttons.
It makes the sidebar tabs consistent with the inserter:
You'll also note in that one, that the text color is now uniformly the same dark gray everywhere in the UI.
It makes all panels the same height, previously some panels were 50px tall, others 48, some arbitrary multiples. It also removes a $panel-padding variable, so as to encourage the grid system instead:
It adds a new small close icon, and uses it for both sidebar and prepublish panels. It also vertically aligns it to the chevrons:
It removes the I/O icons from the form toggle, because they added complexity that wasn't helping convey the active/inactive state:
It fixes so this chevron isn't scaled down:
I would like to do much more to improve things, including do an audit of button style usage in the sidebar — small, secondary, tertiary, labels, etc. — for all blocks, including doing some positioning improvements. But instead of creating one mega PR, I'm breaking it up.
Would appreciate your thoughts.