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.

Sign up for GitHub

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

Jump to bottom

add width control to separator block #16925

Closed
wants to merge 1 commit into from
Closed

add width control to separator block #16925

wants to merge 1 commit into from

Conversation

senadir
Copy link
Contributor

@senadir senadir commented Aug 6, 2019 •
edited
Loading

Description

follow up to #16784
tackles a point from #16483

this PR adds width control to separator block

How has this been tested?

create separator components in master
switch to this branch
see if no problems happens

Screenshots

separator-width

Problems

  • themes using max-width on the hr will have to stop in order for those changes to be visible
  • in renders full-width useless since that style currently doesn't relay on anything to have it 100%

Changes

  • introduce a new starting width of 25% instead of the 100px.

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.

@senadir senadir added [Type] Enhancement A suggestion for improvement. Needs Design Feedback Needs general design feedback. [Block] Separator Affects the Separator Block Needs Dev Note Requires a developer note for a major WordPress release cycle labels Aug 6, 2019
@senadir senadir requested review from youknowriad and kjellr August 6, 2019 14:51
@senadir senadir self-assigned this Aug 6, 2019
@senadir
Copy link
Contributor Author

senadir commented Aug 6, 2019

this PR had alignment support ready but it was removed in the last moment because of clearfix problems, alignment relayed on floating the hr, but using float seems to cause clearing problems, if anyone have any experience with this, please tell me
image

This was referenced Aug 6, 2019
Open
Closed
jorgefilipecosta
jorgefilipecosta reviewed Aug 6, 2019
View reviewed changes
packages/block-library/src/separator/edit.js
import {
InspectorControls,
withColors,
PanelColorSettings,
} from '@wordpress/block-editor';

function SeparatorEdit( { color, setColor, className } ) {
function SeparatorEdit( { color, setColor, className, attributes, setAttributes } ) {
const { width = 25 } = attributes;
Copy link
Member

Choose a reason for hiding this comment

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

Should we create a constant at the top of the file for the default width (25)?

packages/block-library/src/separator/edit.js
} }
/>
<InspectorControls>
<PanelBody title={ __( 'Separator Settings' ) }>
<RangeControl
label={ __( 'Percentage width' ) }
Copy link
Member

Choose a reason for hiding this comment

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

Range control has a initialPosition property https://github.com/WordPress/gutenberg//blob/1e40b281d1db725e512e383b5e00f3b59e333d5a/packages/components/src/range-control/README.md. when the value is unset the property allows the range to be in a give position e.g: 25, but the input is empty to allow the user to differentiate between the state the user set the width to 25 (inline style added) the user did not set any width but currently the default is 25 (no inline style is added).
Would it make sense to use this mechanism here?

@jasmussen
Copy link
Contributor

Similar to feedback in #16928 (comment), I wonder if this should be added to the Separator block, or be reserved for the Divider block (and similar to my comment there, this PR is equally impressive 💥).

This is more in a gray territory though, because the feature definitely applies to the Separator block globally. I'll leave the "needs design feedback" label on, to gather some more thoughts.

@kjellr
Copy link
Contributor

kjellr commented Aug 7, 2019

This is more in a gray territory though, because the feature definitely applies to the Separator block globally. I'll leave the "needs design feedback" label on, to gather some more thoughts.

A hesitancy I have around this one is whether it's actually likely to be used. This seems like the sort of thing that would more ideally be adjusted globally for a site, not individually for each separator block.

As noted in the description, it'll also require some theme patches in some cases. I'm not sure that's entirely worth if if users aren't likely to use this feature. (This may just be a bug, but) this appears to overwrite some defaults provided by theme — for example, the "wide" variant in Twenty Nineteen no longer takes over the full width by default:

Screen Shot 2019-08-07 at 10 04 02 AM

@senadir
Copy link
Contributor Author

senadir commented Aug 7, 2019

@kjellr I completely understand that this might require some changes, it's mostly those themes might need to readjust around those new features (an !important for example might be required).
but as you said, I question the need for such deep customization and a site wide settings does seems more appropriate.
still waiting for feedback from @mtias to see if we should drop this feature or take it to another block

@shaunandrews
Copy link
Contributor

What if we added a drag handle to manipulate the width right from the canvas — in addition to, or instead of, the Range control in the sidebar?

@senadir
Copy link
Contributor Author

senadir commented Aug 7, 2019

@shaunandrews that would be as easy as wrapping the block in a ResizableBox I might do it tomorrow, but I will hold on to see if we should move to a divider or not

@jasmussen
Copy link
Contributor

Probably addition to, as we'll always want to have a purely keyboard accessible interface for anything we do.

@BinaryMoon
Copy link

I'm not convinced this is a good thing. I can't imagine many people wanting or needing fine grained control over the width of a hr, and having this would open up the likelihood of there being inconsistent separator widths across the site.

Having a global width setting that gets applied consistently across the site makes more sense to me.

@mapk
Copy link
Contributor

mapk commented Aug 20, 2019

I think we should hold off on this feature as an individual block setting. I'd agree that this feels much more inline with a global setting instead.

@mapk mapk removed the Needs Design Feedback Needs general design feedback. label Aug 20, 2019
@senadir
Copy link
Contributor Author

senadir commented Aug 20, 2019

@mapk does it make sense to converge it to a shape divider block or just mark this with stale?

@mapk
Copy link
Contributor

mapk commented Aug 23, 2019

@senadir, I'm not sure I see a user for it on a Shape Divider block. Do you have some thoughts on that? I'm okay setting this to "stale" for now.

@senadir
Copy link
Contributor Author

senadir commented Aug 23, 2019

@mapk I have no strong feeling toward this PR and it sister #16928 so we might as well close them to reduce the mental weight? I will circle back to them in the future when we have a solid foundation for global settings.
meanwhile, I'm still on the fence about the shape divider block, how should we go about it

@senadir senadir added the [Status] Blocked Used to indicate that a current effort isn't able to move forward label Aug 27, 2019
@senadir senadir closed this Aug 27, 2019
@youknowriad youknowriad deleted the add/separator-width-control branch September 9, 2019 10:07
@youknowriad youknowriad removed the Needs Dev Note Requires a developer note for a major WordPress release cycle label Sep 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jorgefilipecosta jorgefilipecosta jorgefilipecosta left review comments

@youknowriad youknowriad Awaiting requested review from youknowriad

@kjellr kjellr Awaiting requested review from kjellr

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@Soean Soean Awaiting requested review from Soean

@talldan talldan Awaiting requested review from talldan

Assignees

@senadir senadir

Labels
[Block] Separator Affects the Separator Block [Status] Blocked Used to indicate that a current effort isn't able to move forward [Type] Enhancement A suggestion for improvement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@senadir @jasmussen @kjellr @shaunandrews @BinaryMoon @mapk @jorgefilipecosta @youknowriad