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

Add settings to override theme font sizes #6926

Merged
merged 24 commits into from Aug 23, 2019

Conversation

@telamonian
Copy link
Member

@telamonian telamonian commented Jul 31, 2019

References

#6908
telamonian/theme-darcula#4

Since I released a theme a while ago, I have heard a number of very strong opinions about what the correct size is for various fonts. Since there's clearly no one best solution, I think it's about time we added some general font size settings.

Code changes

Various minor changes to ThemeManager and related code.

User-facing changes

There are now settings for changing the font sizes, and corresponding items in the settings menu and the theme palette category. These settings override the font sizes present in the themes. For good measure, I also added items for toggling the scrollbar theming.

Moved theme-related palette items from Settings category to new Theme category.

Backwards-incompatible changes

None

@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Jul 31, 2019

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@telamonian telamonian marked this pull request as ready for review Aug 7, 2019
@telamonian telamonian requested a review from ian-r-rose Aug 7, 2019
@telamonian
Copy link
Member Author

@telamonian telamonian commented Aug 7, 2019

I'm reasonably happy with this PR as is. Paging @ian-r-rose could you please give this a once-over?

@telamonian
Copy link
Member Author

@telamonian telamonian commented Aug 14, 2019

The major issue that needed review (how/where to validate the CSS overrides) has been resolved (by adding "additionalProperties": false to the overrides JSON schema; see #7004).

The PR should be good to go now. Can I please get a fellow dev to look at/sign off on this? The "Increase/Decrease X Font Size" menu items are working quite nicely (eg they're able to read a theme's default settings and start from there, they have a nice behavior if someone sets their font sizes in units of "em", etc).

@telamonian
Copy link
Member Author

@telamonian telamonian commented Aug 14, 2019

...and now for good measure I've added strict validation of the CSS override values (whereas the keys are handled by the JSON schema validation).

@jasongrout @afshin Does my validation implementation seem reasonable?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Aug 16, 2019

Backwards-incompatible changes

Moved theme-related palette items from Settings category to new Theme category.

Is this a UX change, or an API change? If it is just a UX change, we don't consider it backwards incompatible, and it should be listed in the User-facing changes.

@telamonian
Copy link
Member Author

@telamonian telamonian commented Aug 16, 2019

If it is just a UX change, we don't consider it backwards incompatible, and it should be listed in the User-facing changes.

Makes sense. I only changed the palette items, not the underlying commands, so that should just be a UX change. I've updated the notes in my first post accordingly.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Aug 22, 2019

Code changes look good to me, I am testing this locally now.

Copy link
Member

@blink1073 blink1073 left a comment

I tried changing the font sizes in the settings editor using em but it had no effect. Should this have worked?

{
    // Theme
    // @jupyterlab/apputils-extension:themes
    // Theme manager settings.
    // *************************************

    // Theme CSS Overrides
    // Override theme CSS variables by setting key-value pairs here
    "overrides": {
        "ui-font-size1": "3em",
        "code-font-size": "14px",
        "content-font-size1": "2em"
    },

    // Scrollbar Theming
    // Enable/disable styling of the application scrollbars
    "theme-scrollbars": true
}

packages/apputils-extension/src/index.ts Outdated Show resolved Hide resolved
@telamonian
Copy link
Member Author

@telamonian telamonian commented Aug 22, 2019

I think I know where the em setting may have broken. Not sure about the scrollbars toggle. I'll look into it

@telamonian
Copy link
Member Author

@telamonian telamonian commented Aug 22, 2019

@blink1073 Hmm, now I can't reproduce your trouble with the em unit, either. Annoying. Did the px unit work for changing your font sizes?

I copy/pasted the settings you provided, and then inspected the CSS of a filebrowser item in devtools. The font-size is definitely being calculated as I expected (ie as the product of the font size inherited by the element and the magnitude of the em setting). Could you please try your em settings again and then tell me what it says about how the font-size of a filebrowser item is being calculated?

@telamonian
Copy link
Member Author

@telamonian telamonian commented Aug 22, 2019

Ah, crud. It's another set of these bugs. I just tested the PR again in Firefox, and now I see exactly what you mean, @blink1073

And now I'm kicking myself for not testing out the font size changing stuff in multiple browsers. The old scrollbar theming stuff, which is basically being re-used as is, was tested in multiple browsers, so it's weird that that broke.

@telamonian
Copy link
Member Author

@telamonian telamonian commented Aug 22, 2019

I was validating the font size values against the CSS size property, which apparently is not a valid property in Firefox (but it is in Chrome).

I tested a fix on Chrome/Firefox/Safari and pushed it. In Firefox, the scrollbar theming is partially broken: only the horimzontal scroll bar will change color correctly. Some CSS stuff might need some freshening up.

@telamonian telamonian force-pushed the override-theme-font-sizes branch from d195967 to 997b4fe Aug 22, 2019
@telamonian
Copy link
Member Author

@telamonian telamonian commented Aug 22, 2019

@blink1073 All of the issues you found should now be fixed, so give the PR another test.

The bugs were partially due to a rotten CSS selector, and partially due to the differences between CSS handling in specific browsers. This raises the question of how to write tests for the theming settings stuff, but that can be another PR

Copy link
Member

@blink1073 blink1073 left a comment

Works well in Firefox and Chromium on Linux Mint. Thanks!

@blink1073 blink1073 merged commit f17d70e into jupyterlab:master Aug 23, 2019
9 checks passed
@blink1073
Copy link
Member

@blink1073 blink1073 commented Aug 23, 2019

This will go into the next RC, which I plan to cut tomorrow.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants