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

Changes the editor font-setting in the paragraph block from px to em #2653

Closed
wants to merge 2 commits into from

Conversation

iamgabrielma
Copy link
Contributor

@iamgabrielma iamgabrielma commented Sep 3, 2017

Fixes #2610 , but still looking for feedback if that's the best approach.

Changes the font-setting in the paragraph block from a specific pixel size (16px) to a relative size (1em).

@mtias
Copy link
Member

mtias commented Sep 5, 2017

I don't think we'd want ems here, it gets tricky to control in a component based system.

@iamgabrielma
Copy link
Contributor Author

Makes sense, would you rather recommend to modify the $editor-font-size variable as per the latest commit @mtias ?

The variable is used in 4 more spots, all of them within the editor. Any feedback on the best way to go is appreciated.

Example:
screen shot 2017-09-05 at 18 13 31

@codecov
Copy link

codecov bot commented Sep 5, 2017

Codecov Report

Merging #2653 into master will increase coverage by 0.8%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #2653     +/-   ##
=========================================
+ Coverage   31.38%   32.19%   +0.8%     
=========================================
  Files         177      181      +4     
  Lines        5413     5532    +119     
  Branches      949      971     +22     
=========================================
+ Hits         1699     1781     +82     
- Misses       3139     3170     +31     
- Partials      575      581      +6
Impacted Files Coverage Δ
components/popover/index.js 92.95% <0%> (-2.36%) ⬇️
blocks/editable/format-toolbar/index.js 6.38% <0%> (-1.12%) ⬇️
blocks/editable/index.js 10.31% <0%> (-0.21%) ⬇️
editor/actions.js 41.17% <0%> (ø) ⬆️
blocks/library/button/index.js 26.66% <0%> (ø) ⬆️
blocks/library/paragraph/index.js 33.33% <0%> (ø) ⬆️
components/button/index.js 90.9% <0%> (ø) ⬆️
components/drop-zone/provider.js 1.33% <0%> (ø) ⬆️
components/autocomplete/index.js 89.04% <0%> (ø)
blocks/library/gallery/block.js 11.76% <0%> (ø)
... and 4 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 78dd8b6...1212ce8. Read the comment docs.

@notnownikki
Copy link
Member

Did we reach a decision here? I'm currently changing the font-size slider in #3090 and converting it to use em, but if that's not the right approach...

@karmatosed
Copy link
Member

Closing this as it was discussed in this week's meeting. For now we are sticking with px.

@karmatosed karmatosed closed this Mar 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants