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

[Button Block] Allow for text to wrap #2502

Merged
merged 1 commit into from Dec 22, 2017

Conversation

Projects
None yet
5 participants
@jahvi
Copy link
Contributor

jahvi commented Aug 22, 2017

This is my attempt to fix #2249

Single Line Multi Line
screen shot 2017-08-22 at 9 11 00 pm screen shot 2017-08-22 at 9 11 08 pm

I'm hardcoding the top and bottom padding because I'm not sure if there's a way to get this dynamically based on the $blocks-button__height and maybe the font size?

@jahvi jahvi changed the title Allow for multi line text in button block [Button Block] Allow for multi line text in button block Aug 22, 2017

@jahvi jahvi changed the title [Button Block] Allow for multi line text in button block [Button Block] Allow for multi line text Aug 22, 2017

@iseulde iseulde added the [Type] Bug label Aug 22, 2017

@iseulde iseulde changed the title [Button Block] Allow for multi line text [Button Block] Allow for text to wrap Aug 22, 2017

@anna-harrison

This comment has been minimized.

Copy link

anna-harrison commented Aug 24, 2017

The buttons in the screenshots above look ok to me design wise

/cc @karmatosed

@karmatosed

This comment has been minimized.

Copy link
Member

karmatosed commented Aug 25, 2017

@jahvi interesting, have you tried this on various browsers? My concern is depending on device/browser this may not hold up.

@jahvi jahvi force-pushed the jahvi:update/multi-line-button branch 2 times, most recently from b41f878 to 036bd96 Aug 27, 2017

@jahvi

This comment has been minimized.

Copy link
Contributor Author

jahvi commented Aug 27, 2017

@karmatosed Yes, it's a relatively simple change in its core so it looks good in all browser/devices, here's a quick report (I feel like the text should always be aligned to the center in the frontend though, not sure if there's a reason for it or just an oversight):

Comparison Screenshots

Chrome

Editor Frontend
chrome_b chrome

Firefox

Editor Frontend
firefox_b firefox

Safari

Editor Frontend
safari_b safari

Edge

Editor Frontend
edge_b edge

IE 11

Editor Frontend
ie11_b ie11

Mobile

Android 7 iOS 9
android ios

I also noticed the URL input component overlapped the button in the editor when it had more than one line so I fixed that as well.

My only thought is that the $blocks-button__height variable is only used to calculate the button's border radius now, not the height and line-height as before so I'm wondering if I should get rid of it entirely and hardcode its value.

@youknowriad youknowriad force-pushed the jahvi:update/multi-line-button branch from 036bd96 to 02de796 Dec 22, 2017

@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 22, 2017

Codecov Report

Merging #2502 into master will decrease coverage by 8.87%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2502      +/-   ##
=========================================
- Coverage   39.08%   30.2%   -8.88%     
=========================================
  Files         290     174     -116     
  Lines        6967    5270    -1697     
  Branches     1273     900     -373     
=========================================
- Hits         2723    1592    -1131     
+ Misses       3570    3121     -449     
+ Partials      674     557     -117
Impacted Files Coverage Δ
components/panel/row.js 0% <0%> (-100%) ⬇️
blocks/block-controls/index.js 0% <0%> (-100%) ⬇️
blocks/url-input/button.js 0% <0%> (-100%) ⬇️
components/keyboard-shortcuts/index.js 0% <0%> (-85.72%) ⬇️
blocks/alignment-toolbar/index.js 16.66% <0%> (-83.34%) ⬇️
blocks/block-alignment-toolbar/index.js 33.33% <0%> (-55.56%) ⬇️
blocks/library/categories/index.js 3.22% <0%> (-36.78%) ⬇️
editor/utils/url.js 66.66% <0%> (-33.34%) ⬇️
blocks/library/latest-posts/index.js 10% <0%> (-30%) ⬇️
blocks/url-input/index.js 1.69% <0%> (-18.95%) ⬇️
... and 324 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8cc8cbb...997e551. Read the comment docs.

@youknowriad
Copy link
Contributor

youknowriad left a comment

Rebased this, I think this is good to go 👍

@youknowriad youknowriad force-pushed the jahvi:update/multi-line-button branch from 02de796 to 997e551 Dec 22, 2017

@youknowriad youknowriad merged commit c97ebf8 into WordPress:master Dec 22, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@jahvi jahvi deleted the jahvi:update/multi-line-button branch Mar 4, 2018

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