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

Introducing Inter UI & Fira typefaces #9207

Merged
merged 11 commits into from
Mar 5, 2020
Merged

Conversation

rusackas
Copy link
Member

@rusackas rusackas commented Feb 26, 2020

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

This PR updates the main UI typeface of Superset to Inter UI, and code fonts (e.g. SQL editor) to Fira Code. Both fonts are under the SIL Open Font License, and license information has been added to LICENSE.txt to reflect this, as per ASF guidelines.

This work is a small step toward SIP-34 (#8976).

Note that the Roboto font (not actually used visibly anywhere) was also removed, in both its binaries and LICENSE.txt references.

Also note that Fira Code is an extension of Fira Mono. I included that typeface temporarily for when ligatures are turned off, but decided it's better to just use OpenType features to just turn ligatures on/off, rather than loading a different font entirely.

Fira Code is a font that contains handy ligatures that many people love. However, not everyone might love them, so there's a LESS variable called @use-ligatures to turn them on or off (they're off by default). If/when we move to a CSS-in-JS approach (See SIP-37, issue #9123), we can make this selection more dynamic, switchable via user preference or deployment config.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

SQL Lab BEFORE (Helvetica + Menlo)
image

SQL Lab AFTER (Inter UI + Fira Code, ligatures OFF)
image

SQL Lab AFTER (Inter UI + Fira Code, ligatures ON)
image

Explore BEFORE (Helvetica)
image

Explore AFTER (Inter UI - note that viz components are still using other fonts - in this case, Arial)
image

I've clicked around to other areas, and things are looking OK. Holler if there are any concerns, and I can add additional screenshots.

TEST PLAN

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

REVIEWERS

@etr2460 @mistercrunch

@rusackas rusackas changed the title Switching to Inter & Fira typefaces Introducing Inter UI & Fira typefaces Feb 26, 2020
@rusackas rusackas marked this pull request as ready for review February 27, 2020 06:23
@willbarrett
Copy link
Member

Yaaaaaayyyyy ligature support!

Copy link
Member

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

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

This seems reasonable, but I am concerned about how much extra fonts we'll be sending down the wire on initial page load. Ideally these should be cached, but it's possible people haven't set up cache headers properly so they'd eat an extra megabyte or two on every page load.

I'd recommend a couple changes:

  • Only include a 3 font weights for each font (thin, normal, bold)
  • Update/improve documentation in the config file about how to properly configure static resource caching

@rusackas
Copy link
Member Author

rusackas commented Mar 3, 2020

@etr2460 I shared this concern as I was implementing this... in fact I went down a rabbit hole of writing LESS conditional mixins to only load the @font-faces that were actually in use. In the end though, it turns out every browser since IE8 is smart enough to NOT load @font-face resources that aren't actually in use. So, I added all the font binaries to the repo in case they come in handy, but in actuality, only three of the weights, and one of the formats, might ever actually get loaded. Usually only a couple of those are actually in use by a given view, and they do appear to be cached from what I saw. So despite all the font binaries visible in this PR, the burden on load is actually pretty light.

@etr2460
Copy link
Member

etr2460 commented Mar 3, 2020

ooh, that's good to know. i think this looks fine from my perspective then. Let's make sure we have an action item going forward to standardize on font weights within our less variables file then so that we don't get a weird mix of different types of font stylings (i still think there might be value in limiting the files committed in the repo so no one accidentally uses uncommon font weights, but it's not too big of a deal)

@williaster
Copy link
Contributor

Is there a plan for updating fonts on the charts? like mass migrating to set font: inherit or something?

@rusackas
Copy link
Member Author

rusackas commented Mar 4, 2020

Is there a plan for updating fonts on the charts? like mass migrating to set font: inherit or something?

@williaster My initial thought, for the shorter term, is to just add the new fonts by name to the front of the font stack. Using "inherit" might also work, but could also have ramifications in other (embeded) use cases. By explicitly naming the font, the charts would fall back to their existing font stack when the new font is not available for whatever reason.

Longer term, I think the plugins should follow the CSS-in-JS patterns we use for the main repo, and ultimately pull from the same theme settings (presumably pulled in from Superset-UI). SIP-37 is in need of votes to make that a possibility.

@etr2460 etr2460 merged commit 7d572d9 into apache:master Mar 5, 2020
========================================================================
Third party Apache 2.0 licenses
Third party SIL Open Font License v1.1 (OFL-1.1)
Copy link
Member

@mistercrunch mistercrunch Mar 5, 2020

Choose a reason for hiding this comment

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

Are we sure SIL is apache-comptible? Listed here as "weak copyleft"
https://www.apache.org/legal/resolved.html#weak-copyleft-licenses

@rusackas rusackas deleted the Inter_Fira branch March 5, 2020 06:33
@rusackas rusackas restored the Inter_Fira branch November 16, 2020 21:47
@rusackas rusackas deleted the Inter_Fira branch November 16, 2020 21:58
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.36.0 labels Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.36.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants