Block Bindings: Remove Block Bindings icon from List View, fixes and CSS updates. #59477
Conversation
artemiomorales
added
[Type] Bug
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: -334 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
I'm not sure what is the desired UI here. I can see the paragraph icon, for example, just turns from black to white. Could something like this make sense when it is selected?
@SaxonF @jasmussen Any thoughts what should be done here? |
@SantosGuillamot Hmm that's not right. It should be showing with the purple outline and a white fill: 
I've tested in a different environment and it's appearing as expected. Are you seeing this in the post editor, site editor, or both? Can you clear your cache and restart your environment to see if the issue persists? |
|
Oh, sorry. I was just suggesting another approach that came to my mind based on the current behavior of other icons. My point was that I'd like to get some feedback to ensure the UI we provide is the desired one. |
Ah ok! In that case I'd say that leaving the icon with purple outline and white fill is best, in my opinion. The blue highlight of the selected block already signals a change, and if we also modify the icon, I believe it introduces cognitive dissonance with too many things changing at once and could look like a mistake with inconsistency of the icon across the UI, though I'd like to hear what design thinks 😄 |
artemiomorales
changed the title
gziolo
added
Backport to WP 6.7 Beta/RC
|
Thanks for following up on this. I'd personally love to find some time to explore a couple of more icon variations — the current one is pretty good, but I think we can be even better. That said, responding to this comment — this is actually what I'd expect: the icon not having color there. So the screenshot there is actually what I'd expect. |
This makes sense. So, the final UI should look like: 
Additionally, I believe the SVG paths need to be adjusted. Currently the storybook for the connection icon looks like this: 
|
|
@jasmussen Could you provide us with an SVG file that meets those requirements and renders correctly in the storybook? The one in the Figma file is not quite correct. I tried converting it myself and ask gpt for help but that didn't result in a working code, unfortunately 😢 |
|
Try this one: I think there are further options to improve it, but we can look at that separately. |
|
@jasmussen I have just tested the SVG you shared but it seems it is completely white. If I remove the fill="none" property it looks like this:
It might need to fill the whole SVG, right? I think it can be fixed removing the |
|
I've updated the SVG in this commit. This is how it looks in the storybook right now:
We need to update the CSS logic to match it. Working on it now. |
|
Try this: |
I tried something similar here and seems to work. Thanks for your help @jasmussen |
|
Okay, I've adapted the styles and make some changes based on feedback: Before SVG and styles changes
After SVG and styles changes
We can see that, with the new changes, the SVG in the block toolbar looks wider. I know it can be solved with CSS and stroke-width, but I'm not sure if that's the right approach. I'll take a look if it can be solved other way. Apart from that, I can see that if I enabled the distraction free mode, the icon is not aligned. We must take a look at that as well:
|
Yes, I think this one can be sanctioned, can we have an isolated PR/issue for it to be added to the sanctioned items on the board. |
I have just created a separate PR for that: link. Let me know if that's enough or I should do something else. |
|
That should be fine. Thank you. |
|
@youknowriad Is there any chance we could still get this in? The majority of the changes are CSS whitespace 🙂 The icon in the List view currently looks broken: 
We can also remove the icon in the ListView altogether. |
|
Unfortunately no, I have punted several similar small changes as well. It's just that for RC1, it's only critical things that are allowed. |
I understand, thanks for clarifying! 🙂 |
|
On the parent block front, here's what I saw when testing yesterday: parent.block.movNote: I'm using WP Playground and have run into tons of issues there so perhaps it's a limitation of playground. Tested again using Playground for this PR and WP nightly: Screen.Recording.2024-03-05.at.9.40.45.AM.mp4Another item of feedback shared in the video above-- I'd want the icons in this order: image, block binding, lock. Right now, having the image preview shoved between the two in list view is a bit clunky:
|
This reverts commit 717bbe8.
@annezazu This issue with the parent selector not being visible appears to be a pattern overrides issue and I think is best handled separately — pull request created here: #59621
I've swapped the positions of the icons, but I'm not sure the result is any better. Now the connections icon and lock icon appear very far apart due to all of the icons' different sizes and treatment of white space. Given what @jasmussen mentioned above, perhaps of least resistance is to just remove it for now. Otherwise, Joen do you or perhaps @SaxonF have ideas on how to handle? 
|
|
We also can keep it as is if design thinks it's better. I didn't realize how odd the spacing would look in general. Something to follow up on outside of this release and refine. |
In my opinion, we could just remove it until we figure out what is the best strategy for the UX in this regard. It seems it isn't clear if we should have that icon at all. |
|
That sounds fine. We can revisit after 6.5 |
| stroke-width: 1.5; | ||
| stroke-linecap: round; | ||
| stroke-linejoin: round; | ||
| svg { |
There was a problem hiding this comment.
We can now remove those properties because of the updates to the of the SVG.
michalczaplinski
changed the title
|
I've removed the Icon from the List View and updated the PR description to reflect that 👍 |
|
Most of these are formatting changes. Tested the latest changes and it's looking good to me — the list icon is removed, and the icon still appears in the block toolbar as expected. |
bph
added
the
[Feature] List View
…CSS updates. (WordPress#59477) * Fix bindings icon fill color in list view * Rewrite fill-rule as fillRule to fix console error * Fix connection SVG * Fix fixRule * Adapt styles to new SVG * Workaround for distraction free * Fix: - the list view styles, - the stroke & fill in svg - remove unnecessary CSS - lint:fix the scss files * Display parent selector when focused on pattern override * Move list view connections icon beside lock icon * Revert "Display parent selector when focused on pattern override" This reverts commit 717bbe8. * Remove unused CSS styles * Remove Icon from List View --------- Co-authored-by: artemiomorales <[email protected]> Co-authored-by: SantosGuillamot <[email protected]> Co-authored-by: michalczaplinski <[email protected]> Co-authored-by: jasmussen <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: annezazu <[email protected]> Co-authored-by: SaxonF <[email protected]>
What?
This is a quick follow up to #59185
<path>now instead of 6 of them.fillRuleinstead offill-rule.Testing Instructions
Please follow the instructions in #59185