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 font-size option to cover text. #2397

Merged
merged 3 commits into from Aug 16, 2017

Conversation

Projects
None yet
3 participants
@mtias
Contributor

mtias commented Aug 14, 2017

This PR does two things:

  • Updates RangeControl to have a number input, adds icon support, and moves the label to the top.
  • Adds font-size support to cover text.

font-size-control


cc @jasmussen @karmatosed @melchoyce @westonruter

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 14, 2017

Contributor

Love this feature. Super nice work.

I still think this should be the text block, and not a separate block. But okay to explore separately, if you're okay with merging the two later on.

I'm a fan.

Contributor

jasmussen commented Aug 14, 2017

Love this feature. Super nice work.

I still think this should be the text block, and not a separate block. But okay to explore separately, if you're okay with merging the two later on.

I'm a fan.

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Aug 14, 2017

Contributor

I think if I can get to adding the "clear colors" options and refine the palette accessibility a bit, then we could look at combining next week. I agree in the long term "cover text" should disappear as it's not obvious what it does or when you should consider it over text. (This does make me regret the renaming of text to paragraph, though.)

Contributor

mtias commented Aug 14, 2017

I think if I can get to adding the "clear colors" options and refine the palette accessibility a bit, then we could look at combining next week. I agree in the long term "cover text" should disappear as it's not obvious what it does or when you should consider it over text. (This does make me regret the renaming of text to paragraph, though.)

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Aug 14, 2017

Codecov Report

Merging #2397 into master will increase coverage by 0.09%.
The diff coverage is 60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2397      +/-   ##
==========================================
+ Coverage   26.44%   26.54%   +0.09%     
==========================================
  Files         157      157              
  Lines        4851     4853       +2     
  Branches      819      818       -1     
==========================================
+ Hits         1283     1288       +5     
+ Misses       3015     3012       -3     
  Partials      553      553
Impacted Files Coverage Δ
blocks/inspector-controls/range-control/index.js 100% <ø> (ø) ⬆️
blocks/library/cover-text/index.js 36.36% <60%> (+1.36%) ⬆️
components/dashicon/index.js 4.65% <0%> (+0.74%) ⬆️

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 c8c8078...340f205. Read the comment docs.

codecov bot commented Aug 14, 2017

Codecov Report

Merging #2397 into master will increase coverage by 0.09%.
The diff coverage is 60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2397      +/-   ##
==========================================
+ Coverage   26.44%   26.54%   +0.09%     
==========================================
  Files         157      157              
  Lines        4851     4853       +2     
  Branches      819      818       -1     
==========================================
+ Hits         1283     1288       +5     
+ Misses       3015     3012       -3     
  Partials      553      553
Impacted Files Coverage Δ
blocks/inspector-controls/range-control/index.js 100% <ø> (ø) ⬆️
blocks/library/cover-text/index.js 36.36% <60%> (+1.36%) ⬆️
components/dashicon/index.js 4.65% <0%> (+0.74%) ⬆️

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 c8c8078...340f205. Read the comment docs.

@aduth

aduth approved these changes Aug 14, 2017

The behavior is a bit odd going from "unset" to a specific font size, but I don't think we can (or care to) figure out what the default value is to assign.

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Aug 14, 2017

Contributor

but I don't think we can (or care to) figure out what the default value is to assign.

I originally had the default we use in the editor, and it was alright. If this value comes through the theme add_support definition, we could use that. If the default is the same as assigned, we can avoid adding the styles. But it didn't seem too intuitive, and a theme may have dynamic sizes depending on context. A default may still be desirable but it would have to be client side only (i.e. just assign temporarily if it's not set, but don't mess with the attributes).

Contributor

mtias commented Aug 14, 2017

but I don't think we can (or care to) figure out what the default value is to assign.

I originally had the default we use in the editor, and it was alright. If this value comes through the theme add_support definition, we could use that. If the default is the same as assigned, we can avoid adding the styles. But it didn't seem too intuitive, and a theme may have dynamic sizes depending on context. A default may still be desirable but it would have to be client side only (i.e. just assign temporarily if it's not set, but don't mess with the attributes).

mtias and others added some commits Aug 14, 2017

Add fontSize control to cover text.
Add has-bg class and simplify style declaration.

Update class names for clarity and fix test.

@mtias mtias merged commit 3947282 into master Aug 16, 2017

3 checks passed

codecov/project 26.54% (+0.09%) compared to c8c8078
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@mtias mtias deleted the add/font-size-options branch Aug 16, 2017

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