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

Change font size #446

Merged
merged 7 commits into from
Apr 24, 2020
Merged

Change font size #446

merged 7 commits into from
Apr 24, 2020

Conversation

kinow
Copy link
Member

@kinow kinow commented Apr 20, 2020

These changes close #141

There is a TODO about #335, but for now it's storing the new font size in the LocalStorage (i.e. client-side only).

Under the user profile view, now there is a "Preferences" section, with the Font Size.

  • Vuetify appears to be setting the fixed font-size only on <html> element, and computing the sizes of other elements relative to that value (rem).
  • I've adjusted our code to do the same, except by the Graph component that has some fixed values that I don't dare to change as it would probably cascade the amount of changes necessary and I am not sure if these could be easily fixed
  • Reset sets the value from Vuetify sass variable back to the <html> element
  • The button to increase font size does so with a ratio of 20% for every click, and stores the new value in the LocalStorage
  • The button to decrease does so with a 20% ratio too (decay), and also stores new value in the LocalStorage
  • When the application is loaded, it takes care of setting the <html> element font size back to what is in LocalStorage. If there is nothing in LocalStorage, then it does nothing and the sass variable value is applied as before

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Appropriate tests are included (unit and/or functional).
  • Appropriate change log entry included.
  • No documentation update required.

Will add changelog and tests (unit + e2e) later.

@codecov-io
Copy link

codecov-io commented Apr 20, 2020

Codecov Report

Merging #446 into master will increase coverage by 1.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #446      +/-   ##
==========================================
+ Coverage   46.64%   47.71%   +1.07%     
==========================================
  Files          37       38       +1     
  Lines         879      897      +18     
  Branches       64       64              
==========================================
+ Hits          410      428      +18     
  Misses        456      456              
  Partials       13       13              
Impacted Files Coverage Δ
src/utils/font-size.js 100.00% <100.00%> (ø)

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 2d121b5...903d207. Read the comment docs.

@kinow
Copy link
Member Author

kinow commented Apr 20, 2020

image

image

(There could still be some elements where the change is not applied, e.g. "This is a read-only view of your user" text at the table top seems to be using fixed size. I'll try to tackle some more as I find, but we can also push it to the backlog and fix as we find them I think.)

@kinow kinow self-assigned this Apr 20, 2020
@kinow kinow added this to the 0.2 milestone Apr 20, 2020
@kinow kinow force-pushed the change-font-size branch 2 times, most recently from 2f174aa to ce54b0c Compare April 21, 2020 05:53
src/utils/font-size.js Outdated Show resolved Hide resolved
tests/e2e/specs/userprofile.js Outdated Show resolved Hide resolved
getItem () { return '{}' },
setItem () {}
}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No LocalStorage with JSDOM, interesting too.

@kinow
Copy link
Member Author

kinow commented Apr 22, 2020

Cypress was skipping the newly added tests. In userprofile.js spec, it used one of our JS modules that imported Vuetify's scss file (in JS, not css). That resulted in no errors in Cypress GUI or terminal, and Cypress reporting no tests found.

After searching GitHub and StackOverflow, found some people mentioning things like turning off optimization. Tried it, and that fixed the issue. Now tests being executed. Ran the whole e2e tests locally, everything worked fine.

The issue was similar to this one: cypress-io/cypress#6159

@kinow kinow marked this pull request as ready for review April 22, 2020 22:24
@oliver-sanders
Copy link
Member

Looks good, session storage is a good stepping-stone.

I've part-tested, need a more functional setup to go further. Looks like one element isn't responding, I guess this is because the content isn't wrapped in <p/>, <span/> or whatever:

Screenshot 2020-04-23 at 00 00 27

@kinow
Copy link
Member Author

kinow commented Apr 23, 2020

Looks good, session storage is a good stepping-stone.

I've part-tested, need a more functional setup to go further. Looks like one element isn't responding, I guess this is because the content isn't wrapped in <p/>, <span/> or whatever:

Screenshot 2020-04-23 at 00 00 27

Ah, good idea! I was thinking how to tackle that dangling piece of text that wouldn't resize.

First I wrapped it in a paragraph. But then the font-size applied was the 16px from the vuetify variable (guess it's applied not only to html element).

But then I remembered Vuetify also provides classes for each font-size available, and it's documented in their typography section; and body-1 is the default size, but not fixed as 16px, but instead set as 1rem !important (that's what we get in the computed style).

Pushed one more commit that wraps every piece of text that is not already in a h3 or another element that defines the font-size, using the body-1 class. Now it appears to be working OK.

Thanks for the testing and good suggestion!

Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Some elements of the UI break down with large font-sizes but that is to be expected.

Might want to squash some commits before merging to keep our commit history a bit more readable.

@kinow kinow force-pushed the change-font-size branch from 152ae04 to 3671e0a Compare April 23, 2020 08:23
@kinow
Copy link
Member Author

kinow commented Apr 23, 2020

Might want to squash some commits before merging to keep our commit history a bit more readable.

Rebased, and moved + squashed some commits together.

Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested as working - nice!

@hjoliver
Copy link
Member

hjoliver commented Apr 24, 2020

@kinow - I'll leave it to you to deconflict and merge.

@kinow
Copy link
Member Author

kinow commented Apr 24, 2020

@kinow - I'll it to you to deconflict and merge.

Thank you Hilary!

@kinow kinow force-pushed the change-font-size branch from 3671e0a to 903d207 Compare April 24, 2020 03:48
@kinow
Copy link
Member Author

kinow commented Apr 24, 2020

Rebased on master. Also did a quick smoke test using offline mode, no issues found.

@kinow kinow mentioned this pull request Apr 24, 2020
6 tasks
@hjoliver hjoliver merged commit 836228b into cylc:master Apr 24, 2020
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.

adjustable font size
4 participants