Show "none" as an alignment option and use contextual text to clarify settings #34710
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ntsekouras
added
[Feature] Blocks
ntsekouras
requested review from
youknowriad,
mtias,
oandregal,
gziolo,
ciampo,
jasmussen and
diegohaz
|
Size Change: +4.71 kB (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
ntsekouras
commented
Sep 9, 2021
| ); | ||
| } ) } | ||
| </MenuGroup> | ||
| { ! wideAlignmentsSupport && <div>{ help }</div> } |
There was a problem hiding this comment.
This will also need polishing like the tag to render and styles.
|
Nice work! I see some CSS variables output here:
We need to do the icon changes as well. The existing "block-alignment-center" icon should be repurposed as the new "none" icon (in other words, renamed in the npm library). And then we add a new center icon, this one: Nice work! |
|
Perhaps to explain what's going on — I have this in my theme.json. It might pose a challenge for us 🤔 What, if anything, can we do here? Is referencing CSS variables inside theme.json something we should never do? |
jasmussen
added
the
Needs Figma Update
|
Added a Figma label as a reminder to myself. |
Maybe it won't be that hard to parse: |
mtias
reviewed
Sep 10, 2021
packages/block-editor/src/components/block-alignment-control/ui.js
Outdated
Show resolved
Hide resolved
|
With this being merged: #21892, do we want to have the |
ntsekouras
force-pushed
the
try/new-alignments-control
branch
from
7ccdc69 to
ae1eb5e
Compare
|
You mean when the dropdown is open? |
No, when it's closed. Now I've kept it highlighted in the |
|
This also fixes #30782. |
I don't think that is something we shouldn't do, but would like some thoughts from @oandregal or @jorgefilipecosta |
Ah, yes, that makes sense. I think none of the alignments should have a toggled state when resting on the toolbar? Only on the dropdown. cc @jasmussen |
It's fine as long as the CSS variables used are the ones we generate/control via We have tools to parse the var to the actual value in the |
ntsekouras
force-pushed
the
try/new-alignments-control
branch
from
88b64d3 to
1122d01
Compare
8 tasks
I guess if this PR is ready to merge we can just include a temporary solution using some of the code here and then we refactor this way we keep both PR's moving without blocking each other. |
ntsekouras
force-pushed
the
try/new-alignments-control
branch
from
a655d57 to
a3932db
Compare
I guess some good code review and maybe some rethinking about:
|
youknowriad
reviewed
Sep 24, 2021
| @@ -39,7 +55,8 @@ const BLOCK_ALIGNMENTS_CONTROLS = { | |||
| }, | |||
| }; | |||
|
|
|||
| const DEFAULT_CONTROL = 'center'; | |||
| const DEFAULT_CONTROL = 'none'; | |||
| const help = __( 'Wider alignments are not available.' ); | |||
There was a problem hiding this comment.
I actually think this message is not helpful at all, because it can confuse the user in assuming that wide alignments are not available anywhere while they are container-dependent.
There was a problem hiding this comment.
The copy of this is not finalized: #34597 (comment) and IMO we could definitely could omit that here and add it in a follow up..
There was a problem hiding this comment.
I'm in the opinion that we shouldn't add it at all but 🤷♂️
There was a problem hiding this comment.
This is for cases like within a column where wide doesn't make sense? Let's skip showing anything there. We should only show a message on container where wide/full could be supported but are not.
There was a problem hiding this comment.
@mtias is it okay to skip in this PR? I think it needs more thinking and could be added in a follow up - especially if we try to add some logic to find if a block is a container or not.
youknowriad
reviewed
Sep 24, 2021
packages/block-editor/src/components/block-alignment-control/ui.js
Outdated
Show resolved
Hide resolved
youknowriad
reviewed
Sep 24, 2021
packages/block-editor/src/components/block-alignment-control/use-available-alignments.js
Outdated
Show resolved
Hide resolved
youknowriad
reviewed
Sep 24, 2021
youknowriad
reviewed
Sep 24, 2021
youknowriad
reviewed
Sep 24, 2021
youknowriad
reviewed
Sep 24, 2021
youknowriad
approved these changes
Sep 24, 2021
|
Testing.... Media & Text block Alignment-none.mp4Works nicely! I also tested with columns block with 3 columns inside a Group block. This also worked nicely! Great work! |
|
Thank you all for your help and feedback! |
7 tasks
|
Hey @ntsekouras. 👋🏻 Unfortunately, these changes introduced a regression for the native rendering of |
Oh shoot... Looking at the PR right now!! Thanks @dcalhoun ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Resolves: #34597
Fixes: #30782
This PR is for now a rough POC to update the design of the alignment dropdown to include "none" as an option (it's the same as unsetting). The center icon has been updated, and the previous icon for centering has been repurposed for indicating "None". There's help text for "None" and "Wide", to show the values for the content and wide area layout values.
For themes that do not support wide or fullwide alignments we render some contextual text to clarify the missing buttons:
-- the above description is from the issue
Testing instructions
layout(likett1-blocks) insert aHeadingin a postGroupand change thelayout's widths (contentandwide)Headinginside thisGroupalignment control optionsin bothHeadingblocks.In general I will need help with testing to find out missing functionality/info, especially from folks experienced in themes.
Tasks / Notes
wide alignmentsare not supported.wide alignmentsare supportedwide and contentwidths from somewhere else besideslayout?ToolbarGroup) is affected somehow or needs changescentericon with the new oneuseAvailableAlignmentsintact and incorporate a new helper hookNotes
aligntests also made sure that work as expected intt1 and tt1-blocksas the button context changes. I didn't add to run on both themes though.. Should we run these for both themes? 🤔