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

Fix the More Block input width #8751

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@pento
Member

pento commented Aug 9, 2018

Description

The current behaviour of the More block input element is to expand the width based on the length of the current content. Unfortunately, the current calculation (length + 1) doesn't allow for glyphs that are always going to be more than 1em in width.

This PR changes the calculation to be (length * 3), effectively allowing glyphs up to 3em in width to display correctly. While it is technically possible to construct even wider glyphs through the judicious use of modifiers (eg, "﷽"), this is not to my knowledge an actual meaningful character, so is unlikely to be a real use case.

To avoid unsightly overflow issues for super long labels encountered whilst testing, this PR also limits the max-width of the input.

Fixes #8750.

Screenshots

the more block with a Japanese label

the more block with an overflowing label

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@pento pento added this to the 3.6 milestone Aug 9, 2018

@pento pento requested a review from WordPress/gutenberg-core Aug 9, 2018

@noisysocks

This comment has been minimized.

Show comment
Hide comment
@noisysocks

noisysocks Aug 10, 2018

Member

Is there a reason we don't make the input always 100% wide?

Member

noisysocks commented Aug 10, 2018

Is there a reason we don't make the input always 100% wide?

@pento

This comment has been minimized.

Show comment
Hide comment
@pento

pento Aug 10, 2018

Member

It's existed since the original PR (25cf370). Maybe @youknowriad recalls why?

Member

pento commented Aug 10, 2018

It's existed since the original PR (25cf370). Maybe @youknowriad recalls why?

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 10, 2018

Contributor

I think originally, we had a dashed line on the left/right of the input and a 100% large input was hiding this line. But from the screenshot, the line doesn't seem to be present anymore?

Contributor

youknowriad commented Aug 10, 2018

I think originally, we had a dashed line on the left/right of the input and a 100% large input was hiding this line. But from the screenshot, the line doesn't seem to be present anymore?

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 15, 2018

Contributor

I guess removing the dashed line was a regression and not intended which make me think fixing this issue might more complex than it seems.

I'm removing from 3.6 but feel free to bring back if we reach a resolution.

Contributor

youknowriad commented Aug 15, 2018

I guess removing the dashed line was a regression and not intended which make me think fixing this issue might more complex than it seems.

I'm removing from 3.6 but feel free to bring back if we reach a resolution.

@youknowriad youknowriad modified the milestones: 3.6, 3.7 Aug 15, 2018

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Aug 15, 2018

Member

What about a solution leveraging scrollWidth ?

https://codepen.io/aduth/pen/ZjNwad

Member

aduth commented Aug 15, 2018

What about a solution leveraging scrollWidth ?

https://codepen.io/aduth/pen/ZjNwad

@@ -38,7 +38,9 @@ export default class MoreEdit extends Component {
const toggleNoTeaser = () => setAttributes( { noTeaser: ! noTeaser } );
const { defaultText } = this.state;
const value = customText !== undefined ? customText : defaultText;
const inputLength = value.length + 1;
// Set the input width to 3em per character, plus 1em to allow for empty strings.

This comment has been minimized.

@aduth

aduth Aug 15, 2018

Member

em is a vertical measure, isn't it? Maybe more accurate-ish to use ch ?

https://developer.mozilla.org/en-US/docs/Web/CSS/length#Font-relative_lengths

@aduth

aduth Aug 15, 2018

Member

em is a vertical measure, isn't it? Maybe more accurate-ish to use ch ?

https://developer.mozilla.org/en-US/docs/Web/CSS/length#Font-relative_lengths

@youknowriad youknowriad modified the milestones: 3.7, 3.8 Aug 30, 2018

@karmatosed

This comment has been minimized.

Show comment
Hide comment
@karmatosed

karmatosed Sep 3, 2018

Member

Design wise having this fixed makes sense. Removing design feedback as totally 👍 having a fix.

Member

karmatosed commented Sep 3, 2018

Design wise having this fixed makes sense. Removing design feedback as totally 👍 having a fix.

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Sep 3, 2018

Contributor

We should add the dash line back, it's pretty confusing without it.

Contributor

mtias commented Sep 3, 2018

We should add the dash line back, it's pretty confusing without it.

@youknowriad youknowriad removed this from the 3.8 milestone Sep 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment