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

Change formatting to local em-sizes #777

Merged
merged 1 commit into from Aug 26, 2018

Conversation

2 participants
@signalwerk
Contributor

signalwerk commented Aug 25, 2018

Problem: I think the component-styling looks great. But if I need to place it smaller or bigger in my design I have to change all the default stylings.

Suggested solution: if the styling is on the root-level set by rem (line: 5) and in the component by em I just need to change .DayPicker { font-size: 2rem; } and the whole calendar will scale in proportion to it.

Compatibility: This change should not break any existing styling. People who did manual styling will not be affected, since they reset the default styling. And people who made no changes will not notice, since it's visually equal to the existing codebase.

Sidenote: on Line 88 there was already a em format introduced.

Change formatting to local em-sizes
Problem: I think the component-styling looks great. But if I need to place it smaller or bigger in my design I have to change all the default stylings.

Suggested solution: if the styling is on the root-level set by `rem` (line: 5) and in the component by `em` I just need to change `.DayPicker {  font-size: 2rem; }` and the whole calendar will scale in proportion to it.

Compatibility: This change should not break any existing styling. People who did manual styling will not be affected, since they reset the default styling. And people who made no changes will not notice, since it's visually equal to the existing codebase.

Sidenote: on [Line 88](https://github.com/gpbl/react-day-picker/blob/master/src/style.css#L88) there was already a `em` format introduced.
@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Aug 26, 2018

Codecov Report

Merging #777 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #777   +/-   ##
=======================================
  Coverage   99.84%   99.84%           
=======================================
  Files          15       15           
  Lines         646      646           
  Branches      142      142           
=======================================
  Hits          645      645           
  Misses          1        1

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 d616361...42ebe72. Read the comment docs.

codecov bot commented Aug 26, 2018

Codecov Report

Merging #777 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #777   +/-   ##
=======================================
  Coverage   99.84%   99.84%           
=======================================
  Files          15       15           
  Lines         646      646           
  Branches      142      142           
=======================================
  Hits          645      645           
  Misses          1        1

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 d616361...42ebe72. Read the comment docs.

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Aug 26, 2018

Owner

Good idea, thanks! I think we should switch completely to em (so, even the root element) in the next major release – what do you think?

Owner

gpbl commented Aug 26, 2018

Good idea, thanks! I think we should switch completely to em (so, even the root element) in the next major release – what do you think?

@gpbl gpbl added the v:patch label Aug 26, 2018

@gpbl gpbl merged commit d91416b into gpbl:master Aug 26, 2018

3 checks passed

ci/circleci: checkout-and-test Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing fb66e52...42ebe72
Details
codecov/project 99.84% remains the same compared to fb66e52
Details
@signalwerk

This comment has been minimized.

Show comment
Hide comment
@signalwerk

signalwerk Aug 26, 2018

Contributor

@gpbl that sounds good to me. I just thought I will not rock the boat too heavy 🤓 and keep it compatible with the existing codebase. but I guess it's also great to change the root to em. Feels actually better with em. Then it can be wrapped in an element with defined font-size and no need to overwrite the .DayPicker. Sounds good to me!

Contributor

signalwerk commented Aug 26, 2018

@gpbl that sounds good to me. I just thought I will not rock the boat too heavy 🤓 and keep it compatible with the existing codebase. but I guess it's also great to change the root to em. Feels actually better with em. Then it can be wrapped in an element with defined font-size and no need to overwrite the .DayPicker. Sounds good to me!

@signalwerk signalwerk deleted the signalwerk:rem-em-sizing branch Aug 26, 2018

@gpbl gpbl referenced this pull request Aug 26, 2018

Open

v8 checklist #779

0 of 9 tasks complete

@gpbl gpbl added this to the v7.2.0 milestone Aug 27, 2018

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