Size Change: +3.3 kB (+0.19%)

Total Size: 1.75 MB

_{compressed-size-action}

@madhusudhand as promised I've pushed a different approach to parsing shadow strings in 60c40f3.

While that commit gets all 2040 unit tests passing, it was only quickly hacked together and could definitely do with some refinement. That said, hopefully, it helps unblock the rest of the UI work here, I'm looking forward to seeing the end result!

Some aspects of the updated approach that need looking at are;

• making sure that there is type consistency in parsed values e.g. '0' vs 0
• making it more robust in the face of invalid strings, including extra unit tests for these
• inline comments to explain the various regex patterns used and maybe also the fact that length values must all be together for it to be a valid shadow string
• whether or not extra spaces should be stripped when the shadow strings are parsed or built
• performance, there's likely a more performant approach that this could be refined towards

There's probably more that I haven't noted but as this PR is still in its early days, I'm sure it will all come out in the wash 😄

Flaky tests detected in 44a72d0.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9173073571
📝 Reported issues:

• [Flaky Test] Clicking on inserter while on zoom-out should open the patterns tab on the inserter #61806 in /test/e2e/specs/site-editor/zoom-out.spec.js

@aaronrobertshaw Thanks for the commit. It is working great for all the listed scenarios of valid shadow strings.

making it more robust in the face of invalid strings, including extra unit tests for these

I have considered adding few invalid strings such as {x} {color} {y}.
The above string is invalid in browser perspective as it expects the lengths to be consecutive and there can only be 2, 3, or 4 length values exactly. anything else becomes an invalid shadow string.

Our current approach picks all length values and considers first four of them.
https://github.com/WordPress/gutenberg/pull/60706/files#diff-a9239b1be6e77b3723c92bb93c45928bdd59da74b90dcbe602ab71a44c0154eaR56

I am going to refine the regular expression to pick up the units in order.

Short summary for design review

@jameskoster @WordPress/gutenberg-design

Changes are now ready for the first design review. While it addresses everything as per the suggested designs, following are done differently.

Popover

Design	Actual UI

1. For the color selection ColorPalette core component is used and it comes the the above layout, and differs from the suggested one.
2. x,y position square is not implemented yet in the first iteration. as it requires a new component and needs to be tested for acceptability, it shall be implemented in the next iteration.

❓ What do you suggest as the boundaries for the slider.
In the current revision 0-20px or 0-10rem or 0-100% are used as boundaries.

Preview

Design	Actual UI

A white background is used by taking some inspiration from the block preview component.
❓Please confirm if should be a transparent background.

Shadow options

❓Could you suggest designs for the rename, duplicate options. And for delete do you recommend having a confirmation UI? If yes, could you provide one?

Shadow item labeling

❓When the inset is true, do you think the above labeling makes sense?

Shadows panel copy

Please provide copy.

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.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @jarekmorawski.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Unlinked contributors: jarekmorawski.

Co-authored-by: madhusudhand <madhudollu@git.wordpress.org>
Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org>
Co-authored-by: jorgefilipecosta <jorgefilipecosta@git.wordpress.org>
Co-authored-by: jameskoster <jameskoster@git.wordpress.org>
Co-authored-by: mikachan <mikachan@git.wordpress.org>
Co-authored-by: richtabor <richtabor@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.

x,y position square is not implemented yet in the first iteration. as it requires a new component and needs to be tested for acceptability, it shall be implemented in the next iteration.

Would it make sense to use the AlignmentMatrixControl component before we build a proper one?

Thanks for the ping @madhusudhand, excellent work here, this is coming together nicely :)

For the color selection ColorPalette core component is used and it comes the the above layout, and differs from the suggested one.

Good idea, makes sense.

x,y position square is not implemented yet in the first iteration. as it requires a new component and needs to be tested for acceptability, it shall be implemented in the next iteration.

Also fine, and no rush on this.

What do you suggest as the boundaries for the slider.

0 - 20px seems a reasonable range to begin with – it's easy to tweak this later. I wonder if it's worth capturing negative values? Maybe we try -20 - 20 with 0 as the default?

A white background is used by taking some inspiration from the block preview component.
❓Please confirm if should be a transparent background.

I think it's worth trying the transparent texture so that white shadows are somewhat visible.

❓Could you suggest designs for the rename, duplicate options. And for delete do you recommend having a confirmation UI? If yes, could you provide one?

Could we use DropdownMenu V2 for this menu?

Rename should open a small Modal, same as renaming a page, template, or pattern:

Delete should open a ConfirmDialog, same as deleting a template:

Important: Only user-generated (custom) shadows should be rename-able or delete-able.

Duplicate is a little tricky because we need to think about where to send the user post duplication. I'd be tempted to leave duplication out of this PR, and revisit it separately.

❓When the inset is true, do you think the above labeling makes sense?

Yup, looks good :)

Please provide copy.

Maybe something like: "Manage and create shadow styles for use across the site.".

Some additional points:

• When a theme or default shadow has been edited we probably need to add a 'Reset' action to the menu.
• The – icon for removing shadows should be a Button and include a tooltip.
• When there are no theme shadows we can hide the "Theme" section
• I don't think we need to show the "Custom shadows are empty!" message.

Would it make sense to use the AlignmentMatrixControl component before we build a proper one?

Could be worth a try, but probably follow-up territory.

@jasmussen Here's another proposed top-level navigation item for global styles.

I think we should map out how presets like shadow (and spacing eventually) should be presented in global styles.

Agreed, the current IA is not scaling well. The issue feels a bit pressed by the too-granular "Background image". Going back to #47369, the main instinct there is to separate in two categories, presets and styles. Saxon suggests this:

Presets

• Typography
• Colors
• Spacing
• Layout

Styles

• Elements
• Blocks

"Shadows" is perhaps not the best term, but since it's a set of presets, the above heuristic would provide a structure for it at the top, alongside Colors and Typography.

Layout is a bit tricky since it's in-between presets (through inheritance of block spacing and content widths), and styles for the current page (padding). I wonder if we could split that out, clean it up once and for all. The following is a quick idea intended to inspire better ideas:

Presets

• Typography
• Colors
• Shadows
• Spacing

Styles

• Site
• Elements
• Blocks

"Background image" would go in "Site". As would "Padding", currently sitting in Layout. Block spacing would move from Layout to Spacing, along with other presets eventually. "Site" could also be a place where Border could be added.

The separation makes sense, but unsure about "Styles" and "Presets" as headings. Would a shadow be a style or a preset... it's kind of both? The headings might not both be necessary, the labels are generally self-explanatory.

Either way, probably a discussion for #53483. Saxon shared some interesting ideas there already.

Would it work without subheadings? Maybe, though I think they make sense insofar as for shadows, you are only editing the presets there. You apply them (style your blocks) elsewhere.

The subheadings also consider that "Blocks" is currently separated by both a line and introductory text. We can keep it that way.

The thing is, we do need to make some progress for 6.6, especially for "Background image" which is so far removed from its context. #53483 is probably parallel but less likely to make it for 6.6. Unless we have better ideas, we should move "Background image" under "Layout".

I guess it tripped me up because shadows can be both a style, and a style preset. Maybe I'm too used to the 'styles' concept in Figma.

"Background" and "Layout" being grouped together under "Site" seems worth a try.

Hi @madhusudhand,
It seems that although custom shadows are stored:

They are not shown as one of the available options to pick:

There is also a small issue where if the shadow is too big it may make it impossible to correctly preview the other shadows.

For colors, we show the multiple origins of colors:

Should we do the same for shadows?

Are there any plans to allow to pick a specific shadow in a given place besides the preset ones?

Not for this PR but, Would it make sense when picking a color for the shadow show the preset color palettes? If a preset color is selected the shadow would reference it by its variable making the shadows dynamically change when the colors change.

Thanks for the excellent reviews. Bringing this feature in and more suggestions/issues will be addressed in followup PRs.

I am sorry, I missed copying props.