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

Focal point picker -use the input-number class and styles already available #13709

Conversation

m-e-h
Copy link
Member

@m-e-h m-e-h commented Feb 6, 2019

Description

Replaces the class on the number input from components-text-control__input to components-range-control__number.

Also removes the styles for that input since components-range-control__number already has number input styles.

This keeps styles consistent with similar controls like "Background Opacity" and the "Columns" number range.

Visually looks the same. Might be a pixel or two different but it is consistent now.

focalnum

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.

@@ -211,7 +211,7 @@ export class FocalPointPicker extends Component {
<div className="components-focal-point-picker_position-display-container">
<BaseControl label={ __( 'Horizontal Pos.' ) }>
<input
className="components-text-control__input"
className="components-range-control__number"
Copy link
Member

Choose a reason for hiding this comment

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

We should never assume a class name of another component, as it will most certainly cause breakage in the future.

If it's meant to be a range control, it should be implemented as a <RangeControl> component.

Otherwise, its styles must be defined locally, and the class name assigned following the coding guidelines:

https://github.com/WordPress/gutenberg/blob/master/docs/contributors/coding-guidelines.md#naming

@m-e-h
Copy link
Member Author

m-e-h commented Feb 6, 2019

Thanks @aduth! That makes perfect sense and is sort of what I was thinking when I began working on #11555 .
But I think that may have mixed me up a little since the Font Size number input was using the components-range-control__number class outside of the <RangeControl> component.

I also figured components-range-control__number from the RangeControl component made as much or more sense here as components-text-control__input from the TextControl component, which is the current situation.
That rational may have been a little irresponsible.

I'm also used to a more functional CSS methodology to styling.
I'm learning though that it may not always work in this project. Perhaps my approach to #11555 is off?

@aduth
Copy link
Member

aduth commented Feb 6, 2019

But I think that may have mixed me up a little since the Font Size number input was using the components-range-control__number class outside of the <RangeControl> component.

Yes, FontSizePicker is wrong as well then.

I'm also used to a more functional CSS methodology to styling.
I'm learning though that it may not always work in this project. Perhaps my approach to #11555 is off?

I'm not entirely sure I follow the question. On its own, the changes from #11555 don't strike me as alarming, but it all depends how this transpires once FontSizePicker is fixed to stop appropriating the .components-range-control__number class.

@gziolo gziolo added the [Block] Cover Affects the Cover Block - used to display content laid over a background image label Feb 7, 2019
@gziolo gziolo added this to the 5.1 (Gutenberg) milestone Feb 7, 2019
@gziolo gziolo added the [Type] Code Quality Issues or PRs that relate to code quality label Feb 7, 2019
@m-e-h
Copy link
Member Author

m-e-h commented Feb 7, 2019

I'm not entirely sure I follow the question.

Yeah, I was kinda thinking aloud there. To answer my own question, my thinking was a little off. I was trying to cut down on duplicate code by re-using class-names with a predictable set of styles attached. While the results would be good, it isn't really inline with the coding guidelines.

So, in light of discovering components like Focal Point and Font Size, both re-inventing the number input and misusing other control classes, my "new thinking" tells me that we need a <NumberControl> component. Right?

I'll put something together for that and see how it works out.

@chrisvanpatten
Copy link
Contributor

@m-e-h I think that's a great approach; a generic NumberControl that could be re-used in all these instances would be really wonderful.

@m-e-h m-e-h mentioned this pull request Feb 13, 2019
5 tasks
@youknowriad youknowriad removed this from the 5.3 (Gutenberg) milestone Mar 18, 2019
@youknowriad
Copy link
Contributor

Hi There! I've triaging PRs today and it seems this one is stale. I'm going to close now as it doesn't seem like the desired direction. Thanks for your efforts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Cover Affects the Cover Block - used to display content laid over a background image [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants