Open matiasbenedetto opened 2 days ago
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 props-bot
label.
If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
Co-authored-by: matiasbenedetto <mmaattiiaass@git.wordpress.org>
Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org>
Co-authored-by: richtabor <richtabor@git.wordpress.org>
Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org>
Co-authored-by: mtias <matveb@git.wordpress.org>
Co-authored-by: jasmussen <joen@git.wordpress.org>
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.
Size Change: +450 B (+0.02%)
Total Size: 1.82 MB
Filename | Size | Change |
---|---|---|
build/block-editor/index.min.js |
255 kB | +450 B (+0.18%) |
I'm not feeling the popover + popovers. Perhaps we just iterate through style variations when you select the control? to not further block the content?
May be nice to use the current style's background color in the control, like this:
May be nice to use the current style's background color
That seems to be pretty specific to variations with background colors specified and the variation can be any style. Could it be confusing to change that color when using a variation with a background color and not when using variations changing other styles/settings?
That seems to be pretty specific to variations with background colors specified and the variation can be any style. Could it be confusing to change that color when using a variation with a background color and not when using variations changing other styles/settings?
No, I don't think it'd be confusing if it does have a background color and it's used here.
I'm not feeling the popover + popovers. Perhaps we just iterate through style variations when you select the control?
Do you suggest removing this part entirely?
Do you suggest removing this part entirely?
If the suggestion is indeed to add a shuffle styles
here, I don't think it's great. There were reasons we removed the shuffle
from patterns to change design
. Maybe a list like the change design
could be considered.
There were reasons we removed the shuffle from patterns to change design
What were those reasons?
Maybe a list like the change design could be considered.
You mean sorta like the current state of this PR? Maybe just without the preview popover?
What were those reasons?
They were too random, also a user would need to rotate all of them to go back to one they liked, etc..
You mean sorta like the current state of this PR? Maybe just without the preview popover?
I mean like this:
They were too random, also a user would need to rotate all of them to go back to one they liked, etc..
I don't think the "too random" issue is a problem with the section styles being rotated.
I can agree that having to rotate all section styles to get back to the default is a bit annoying though.
You mean sorta like the current state of this PR? Maybe just without the preview popover?
I mean like this:
I see what the "Change design" list is. I was trying to understand what the suggestion was here for section styles.
Is the suggestion that the list contains previews of the block with the different section styles applied? So, no list of buttons like this PR currently has? If I'm now following you correctly, sounds like it's worth a shot.
For discussion purposes in the meantime, here's a quick diff that can be applied to see what cycling section styles through a single button would feel like.
https://github.com/user-attachments/assets/e7cc8a44-3861-482a-a14d-7418c04faa8e
Is the suggestion that the list contains previews of the block with the different section styles applied?
Sorry for not being clear enough. Yes, exactly that.
If the suggestion is indeed to add a shuffle styles here, I don't think it's great. There were reasons we removed the shuffle from patterns to change design. Maybe a list like the change design could be considered.
I think we start here first, then explore a similar pattern to "Change design", but with block previews of the actual pattern.
I committed the diff proposed by @aaronrobertshaw (Thanks!)
I think we start here first, then explore a similar pattern to "Change design", but with block previews of the actual pattern.
In that case, I think the PR is good to go. We can implement the list of styles applied to the pattern as a follow-up.
As per my https://github.com/WordPress/gutenberg/pull/67140#pullrequestreview-2447221592, can we please add proper test instructions?
Instructions and video updated in the PR's body.
Back to the stroke width, perhaps our design gurus could shed some light on what the exact stroke width should be. It might then be possible to enforce that as well?
I think the stroke should not be defined. At least, that's what I observed in other filled library icons. Examples: https://github.com/WordPress/gutenberg/blob/trunk/packages/icons/src/library/cancel-circle-filled.js https://github.com/WordPress/gutenberg/blob/trunk/packages/icons/src/library/help-filled.js
I submitted a PR adding the icon to the library: https://github.com/WordPress/gutenberg/pull/67202
The main aspect of the diff that was only for demo purposes was the temporary SVG used as the icon.
I think the icon should not stop us to merge this. We can replace it in a follow-up PR with the library version if https://github.com/WordPress/gutenberg/pull/67202 gets merged.
Thanks for the iterations @matiasbenedetto 🚀
If @richtabor and our design gurus are happy with the interim icon, I'm ok with moving forward as is. That said, it looks like https://github.com/WordPress/gutenberg/pull/67202 already has a code review approval. If it is that close we might as well push that over the line first and tweak this before merging. I don't think it holds up landing this greatly.
What?
Add section styles switch button in block toolbar in zoom-out mode.
Why?
Fixes: https://github.com/WordPress/gutenberg/issues/66465
How?
Adding the button to the toolbar.
Testing Instructions
Screenshots or screencast
https://github.com/user-attachments/assets/ede56aec-a6db-48f1-9f73-aea007004e31