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

Inter UI Font Family #1402

Merged
merged 14 commits into from
Jan 16, 2019
Merged

Inter UI Font Family #1402

merged 14 commits into from
Jan 16, 2019

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Jan 3, 2019

This PR adds the Inter UI font family to the font family stack. It links to a .css that then links to the actual font files. I've also added a few more font weights as well (making sure the K6 theme remained the same).

Before and after

K6 theme (should look the same)

IE

Even IE looks better

Checklist

  • This was checked in mobile
  • This was checked in IE11
  • This was checked in dark mode
    - [ ] Any props added have proper autodocs
  • Documentation examples were added
  • A changelog entry exists and is marked appropriately
    - [ ] This was checked for breaking changes and labeled appropriately
    - [ ] Jest tests were updated or added to match the most common scenarios
    - [ ] This was checked against keyboard-only and screenreader scenarios
    - [ ] This required updates to Framer X components

@snide
Copy link
Contributor

snide commented Jan 3, 2019

For designers.... pease leave a comment saying you are either OK with this, or are aware of it and will commit to managing your own theme for a font-change.

re: from our meeting today. Setting up all the designers as a reviewer on this. There's still work to do on the PR (figuring out code fonts), but I want to make sure everyone is aware and OK with us making this kind of change to EUI master.

What this means

  1. EUI master will use Inter UI as a font rather than the system stack
  2. This will not effect 6.x kibana because it has its own theme using Open sans.
  3. K7 will drop the theme and start using EUI master, we'll manually add Inter UI font files similar to open sans to master in Kibana but not backport it to 6.x. Dashboards might have small breaks because of font sizing differences and screenshot tests might need to be updated.
  4. Anyone else using EUI master will need to decide if they should overwrite the usage (by changing the font families locally in their app) or adopting Inter UI. This could effect Cloud and Swiftype UIs.

What would be nice

I'd love us to all use the same thing if possible and that you don't make a font theme. It just makes maintenance much easier. Kibana requires SOME sort of font decision because its content is often user-defined and absolute heights and widths depend on it (which leads to breaks from browser to browser).

cc @timroes, @epixa and @stacey-gammon for visibility.

@ryankeairns
Copy link
Contributor

LGTM, very similar font yet more readable.

@gjones
Copy link
Contributor

gjones commented Jan 3, 2019

If we're moving to Inter UI then I think we'll probably be ok with it in Cloud as it's similar enough to SF.

As a side note of little consequence, I think my personal opinion is that EUI, as an open source framework, certainly can and should have a default font, but should also allow users to easily opt out of that default and fall back to system fonts.

@snide
Copy link
Contributor

snide commented Jan 3, 2019

As a side note of little consequence, I think my personal opinion is that EUI, as an open source framework, certainly can and should have a default font, but should also allow users to easily opt out of that default and fall back to system defaults.

Yep, for those that need to simply changing the following variables would do it. I think that's fairly painless, but it is something we might want to mention in the readme.

$euiFontFamily: -apple-system, BlinkMacSystemFont, 'Segoe UI', Helvetica, Arial, sans-serif, 'Apple Color Emoji', 'Segoe UI Emoji', 'Segoe UI Symbol' !default;
$euiCodeFontFamily: 'SFMono-Regular', Consolas, 'Liberation Mono', Menlo, Courier, monospace !default;

@@ -1,8 +1,11 @@
// sass-lint:disable no-url-domains, no-url-protocols

@import url('https://rsms.me/inter/inter-ui.css');
Copy link
Contributor

@snide snide Jan 3, 2019

Choose a reason for hiding this comment

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

Unfortunately we can't do this. It won't work in Kibana. We should just include a <link> stylesheet in the EUI docs and document how to include it. Users that don't have it will fall back to system fonts per the font stack.

Copy link
Contributor

Choose a reason for hiding this comment

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

On the Kibana side we'll manually add the physical font files to the build there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I move the CSS import, but I'm stumped on where to document how to add them.

ce10a30#diff-227845547db976d1b4acdc5a25f22189

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the consuming EUI doc is the most likely candidate.

https://github.com/elastic/eui/blob/master/wiki/consuming.md

Copy link
Contributor

@zumwalt zumwalt left a comment

Choose a reason for hiding this comment

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

LGTM. We will likely override with system fonts in Swiftype until we can spend some time adjusting non-EUI things.

@stacey-gammon
Copy link
Contributor

Has anyone tested this change with reporting? I just know we had issues before with fonts and reporting, so might be worthwhile to make sure this runs correctly when the kibana server is running on linux, or a system with possibly fewer font packages installed, and compare the output from the browser to what is generated in the font.

@cchaos
Copy link
Contributor Author

cchaos commented Jan 4, 2019

@stacey-gammon, Kibana will still run on Open Sans for the time being. Since Open Sans is not a font family that's distributed with any OS's, what does reporting currently use for fonts? I would think this would behave the same way just with a different "preferred" font, but will fallback to any system fonts declared.

@cchaos
Copy link
Contributor Author

cchaos commented Jan 4, 2019

Added iA Writer as mono font:

@cchaos
Copy link
Contributor Author

cchaos commented Jan 4, 2019

Ha, faked you out, using Roboto Mono instead

@timroes
Copy link
Contributor

timroes commented Jan 10, 2019

Pinging @joelgriffith and @tsullivan regarding reporting on this one. Even though that won't immediately change anything around reporting just merging that in, I wanted to rise awareness of that upcoming change in Kibana potentially. Also I think this should make things better for reporting, since we should not be so much dependent on local installed fonts anymore?

@joelgriffith
Copy link
Contributor

Thanks @timroes for pointing this out. I think the only issue is that having some OS font installed is still a requirement, otherwise Chromium panics and fails to start.

That being said it'll hopefully make delta tests and consistency much better now

@tsullivan
Copy link
Member

I agree with @joelgriffith on this. When fonts are the cause of the failure, as I've seen it, it's because the OS doesn't have a "default" font registered. Installing the dependent packages takes care of that.

@snide
Copy link
Contributor

snide commented Jan 11, 2019

Just to set a gameplan for this for everyone. Let's assume design will merge this next week (which shouldn't effect K6 kibana) but will effect Clould (cc @gjones). Then we'll make a change to master Kibana so this is a default we have available for testing purposes. We'll try to plan this around the new nav going in so that tests (if they break) will be all at once and we'll still have the months till 7.0 GA to address them.

Give a yell if that does not sound good.

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Small suggestion to docs. I might suggest including something similar in the fonts stack section of the Sass guidelines page.

wiki/consuming.md Outdated Show resolved Hide resolved
@cchaos cchaos merged commit 0b6110e into elastic:master Jan 16, 2019
@cchaos cchaos deleted the inter-ui branch January 16, 2019 20:44
@snide snide mentioned this pull request Jan 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants