Skip to content
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

List: Add border support #63540

Merged
merged 3 commits into from
Aug 15, 2024
Merged

List: Add border support #63540

merged 3 commits into from
Aug 15, 2024

Conversation

aaronrobertshaw
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw commented Jul 15, 2024

Part of: #43241

What?

Adopts border support for the List block

Why?

  • Offers greater design flexibility
  • Improves consistency in design tooling with other blocks

How?

  • Adopts all the available border block supports
  • Makes the border controls optional to match the dimension controls
  • Defines a custom selector so global styles only target top-level List blocks

Testing Instructions

  • In the site editor, add a List block with some items including a nested list or two, to a page
  • Style List blocks via Global Styles and confirm the editor and frontend display correctly
  • Override the global styles on the block instance and confirm they display appropriately in the editor and frontend
  • Confirm block instance styles apply correctly to nested List blocks

Screenshots or screencast

Screen.Recording.2024-08-09.at.7.57.40.PM.mp4

@aaronrobertshaw aaronrobertshaw added [Type] Enhancement A suggestion for improvement. [Block] List Affects the List Block labels Jul 15, 2024
@aaronrobertshaw aaronrobertshaw self-assigned this Jul 15, 2024
Copy link

github-actions bot commented Jul 15, 2024

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: aaronrobertshaw <[email protected]>
Co-authored-by: andrewserong <[email protected]>
Co-authored-by: carolinan <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this is testing well for me and according to the PR description 👍

Something I noticed while adding styles in global styles is that nested list items use a List block themselves, so setting a List block to have a border in global styles also means it'll apply to children:

image

In sites that do that, it's still possible to override the child list block by setting it to explicitly have a 0 width border:

image

For the use case of this sort of border — i.e. a theme that wishes to set a border on the outer list but not apply to the inner list items, would that be a good use case for someone to add a block style variation instead? If so, I imagine this PR will enable that, too. In any case, these are just thoughts off the top of my head while playing around with this... I don't think it's a blocker for this PR.

LGTM! 🚀

@aaronrobertshaw
Copy link
Contributor Author

Thanks for the review @andrewserong 🚀

As there is no hurry on landing this, I'll find some time to try updating the selector for the border support to see if it can be made to target the parent list block only. If possible, we can then choose which feels the most natural.

@carolinan
Copy link
Contributor

carolinan commented Jul 17, 2024

When adding the wp-block list class to the block, I brought this up, that the same class would be used for the nested lists, and that it would not be possible to style only the parent. But there was no support for adding separate classes or a second class to the nested item.

@aaronrobertshaw aaronrobertshaw force-pushed the add/list-border-support branch from 2adb903 to 2fa7e3a Compare August 9, 2024 13:01
@aaronrobertshaw
Copy link
Contributor Author

Thanks for the extra context @carolinan 👍

Leveraging the Selectors API, it's pretty trivial to make the Global Styles target only top-level List blocks. In 2fa7e3a I added a selector for the border styles that uses :not(.wp-block-list .wp-block-list) to prevent matching on nested lists.

It would be possible to give other features (e.g. spacing) the same treatment but spacing, typography, etc. do seem more "global" than say borders.

The video below shows me fumbling around the first time I tested out the latest changes. I think it does the job. The global style only adds the border to the outer List. The block instance styles can still override the values of global styles and also be used to add borders to inner blocks if so desired.

Screen.Recording.2024-08-09.at.7.57.40.PM.mp4

would that be a good use case for someone to add a block style variation instead?

Whichever way we go a block style variation could be created to achieve the desired result.

If we choose to only apply global styles to the outer List block, then a block style variation for Lists could define a border on inner List block types. The result being borders on all lists.

If we go with the original approach in this PR, the block style variation would just define border style none or border width 0 etc on inner List blocks.

Let me know what you think of the current behaviour. I'm probably leaning towards landing that approach.

Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know what you think of the current behaviour. I'm probably leaning towards landing that approach.

Similar to the work over in #63541, I agree, and in testing this is working nicely for me, with the border applied to the outer most list:

image

I tested the following:

✅ Setting borders for the list in global styles applies only to the outer-most list, and not nested lists
✅ Overriding at the individual block level works as expected
✅ Individual lists and nested lists can still use block-level styles for borders
✅ Tested in the site editor, site frontend, and post editors

LGTM! 🚀

@aaronrobertshaw aaronrobertshaw merged commit e6057ce into trunk Aug 15, 2024
62 checks passed
@aaronrobertshaw aaronrobertshaw deleted the add/list-border-support branch August 15, 2024 03:19
@github-actions github-actions bot added this to the Gutenberg 19.1 milestone Aug 15, 2024
@carolinan
Copy link
Contributor

Perfect, thank you

@artemiomorales artemiomorales added the [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi label Aug 23, 2024
@fabiankaegy fabiankaegy mentioned this pull request Oct 1, 2024
97 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] List Affects the List Block [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants