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

Add supprt for line thickness/Circle size in separator block #16928

Open
wants to merge 3 commits into
base: master
from

Conversation

@senadir
Copy link
Contributor

commented Aug 6, 2019

Description

follow up to #16784 & #16925
tackles a point from #16483

this PR adds size control to separator block (thickness of line & size of circle)

How has this been tested?

using Gutenberg starter theme
create separator components in master
switch to this branch
no problems happens

Screenshots

separator:size-control

Problems

  • while this doesn't deprecate anything, I'm feeling like it's a bit fragile and need some testing, I've done some testing but I didn't have time to test on other themes.

Changes

  • it moves dots style from the :before element to the parent element, for eiser overriding
  • it uses getActiveStyle function introduced in BlockStylesI just copied & past it since I can't reference it directly and it's not exported.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

Can't the thickness be the same thing for all style variations (same as dot size), why specific options for each style variation?

@senadir

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

@youknowriad I interpreted as that they don't scale in the same way (a 20px line is too wide, and 2px circle is barely visible if not). and because conditional option was required when we want to adjust the spacing since spacing doesn't make sense in line.

I could argue against myself here and say that we can use spacing & size as one value and it would make sense.

I can refactor them to use the same option, this should, in theory, reduce a lot of redundant code, but the scaling thing will still persist, but I don't believe it's that of a big a deal.

what's your call Riad?

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

I don't like having specific options per style variations. Like their name suggest, style variations are CSS classes that changes the design but shouldn't affect the behavior.

I'd love thoughts from others @mtias @kjellr @jasmussen @mapk

@senadir

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

it's mostly a fundamental problem in how the dots style & line style are built, while most other blocks styles change in a small way, the way they're built is the same.
but for the separator, it's just a cosmetic block, and the dots style variation, doesn't change change style only but also introduce a new element via the :before, it's still an element that is styled and targeted in a different way, so I feel it's really hard to keep it consistent with the general vision of styles.

I feel like I'm missing something maybe, I would love any thought on this one

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

I really appreciate this work, and the demo is impressive.

However I think the way we add these features should be generic to the separator block, and not any of the individual styles. As Riad alludes to, any theme can redesign the separator to be completely different, and are likely to, and if they go further they can even redesign each variation.

There are some efforts underway on a dedicated "Shape Divider" block here: #16351, which add somewhat similar features, but in a way that is less likely to be completely coopted by a theme. Do you think the efforts here are better applied there?

@senadir

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

Do you think the efforts here are better applied there?

I'm having a hard time understanding the difference between a Divider block & a Separator Block, why should we limit the separator block while augmenting a new block that is essentially Separator++

I'm not against moving those styles to the Divider, but how will that solve the style inconsistency?

I would love some feedback from @mtias as well, since he's the one that created the issue and has a vision for it.

since you're much more senior than me @jasmussen, I trust that you guide me to the correct way to implement it

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

I'm having a hard time understanding the difference between a Divider block & a Separator Block, why should we limit the separator block while augmenting a new block that is essentially Separator++

This is a good observation, and one that normal users are likely to struggle with also. This is something we should keep top of mind as we continue with the separate Divider block. But there is a semantic difference which reinforces the idea of separate blocks for this. One is essentially an <hr /> tag with optional CSS classes, and can serve an HTML semantic purpose even for users that load a page without a stylesheet for accessibility purposes. The other — the divider — is purely decorative, which carries with it its own accessibility best practices.

I can see some of the confusion alleviated by a user browsing the block library and seeing previews for both the separator and the divider, and picking the parent style based on that. In some future it would be fun to offer pre-built sections that contain multiple blocks pre-configured — this would likely help further.

Thank you for your kind words, and of course, ping me any time and I'll help to the best of my ability. Your contributions are delightful!

@jasmussen jasmussen referenced this pull request Aug 7, 2019
4 of 5 tasks complete
@mapk

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

Thank you for this wonderful work!

I'd agree that having separate settings for separate style variations is getting a bit too meta for me. I love the explorations and I think they've helped me realize that in relation to the Shape Divider PR, we may want to limit settings here in the Separator.

I like the settings that allow me to change all the style variations at once, like the color settings. Anything beyond that feels like plugin territory.

@BinaryMoon

This comment has been minimized.

Copy link

commented Aug 9, 2019

I feel the same here that I felt on the width setting (#16925 (comment)) - I think this would be better as a global setting rather than a per block setting.

I also worry that all of these settings will make theming blocks more difficult since we will need to override/ accommodate these custom things, which could be hard or involve adding many !importants.

@senadir

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

@BinaryMoon I always felt that themes should respect the choices made in the editor, not vice versa, theme should provide a default, backup option, but the final visual design should be up to the author, this is just my way of thinking, not Gutenberg way of thinking as far as I know, I guess this could stay open until there is a consistent feedback into how should I shape this (mostly divider) but yeah, I totally convinced that a seperator shouldn't need those things.
as for global settings, there isn't any of them now
I will not merge this PR until further notice

@BinaryMoon

This comment has been minimized.

Copy link

commented Aug 9, 2019

@senadir

I always felt that themes should respect the choices made in the editor

I agree, but I don't think the editor should have too many choices. I like the WordPress philosophy of 'Decisions, not options'. You choose a theme because you like the design so overriding the design and breaking the carefully considered aesthetics doesn't sit well with me.

as for global settings, there isn't any of them now

I know, but I've seen them mentioned a few times recently, which suggests to me they will be coming.

Either way, I'm just expressing my thoughts/ voicing my concerns :)

@senadir

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

@BinaryMoon Absolutely I agree with that philosophy, as for global settings, no one is working on it as far as I know, there has a been a lot of talk, I'm doing a lot of research into it, but since there is no consist way yet to to implement it, it's on hold, the idea is there since the first day of gutenberg, read here #1224

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.