Skip to content

Inter UI and roboto mono font stack. Remove K6 theming. We did it - #29152

Merged
snide merged 32 commits into
elastic:masterfrom
snide:k6/is_dead
Feb 1, 2019
Merged

snide merged 32 commits into
elastic:masterfrom
snide:k6/is_dead

Conversation

@snide

@snide snide commented Jan 23, 2019 •

Copy link
Copy Markdown
Contributor

Summary

This PR does the following things:

  1. Removes references to K6 EUI, using the default UI theme in its place
  2. Removes open sans from Kibana. Replaces it with Inter UI and Robot Mono
  3. Licenses added for fonts; readme and notes added for usage

For outside team pings

  • In security some font sizing was changed. Snaps were updated as well.
  • In canvas I moved the markdown file to ui/public. We have a few different markdown files, and the Canvas one is the best. It is unchanged from the canvas usage and will not cause breaks there.
  • In APM snapshots were updated due to the EUI update.

Notes

I included the full stack of files that Google fonts and Inter UI provided. In many cases these files will not be used (either because a variable file is loaded, or we're not calling the font-face directly), but generally having lived in the Kibana font code for 2 years I've found it prudent to include the full set since these are versioned and you never know when you'll need them. It gives us some room should we need it later.

Testing

I tested this on Chrome, Firefox, Safari and IE11. Firefox has the best tooling for fonts if you're looking to get designy with them. I did not do a full visual check of this stuff. I think @cchaos, who is more familiar with these fonts will do a better job there. But they are rendering at the weights I expect.

Stuff to check

Because the fonts are different, anything that may have used a fixed width or height based on "eyeball" values has the potential to break. For example, if you sized a column because "that looks about 300px of content" rather than making a calculation based on variables, that column may no be 300px.

K6's base font size was 14px. K7s is 1rem/16px. In some cases you might see subtext content (like subtitles) looking large. The fix is always to just use EuiTextand apply a smaller size if that's what you think looks better. I tried to get as many instances of those in this PR as I could.

Breaking change

This is a breaking change to dashboards for 7.0. It will cause pixel level height differences vs K6 because of differences in font size and weight. We'll need to update the baseline screenshots to match.

image

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@snide snide added the v7.0.0 label Jan 23, 2019
@snide
snide requested a review from a team as a code owner January 23, 2019 04:52
@snide

snide commented Jan 23, 2019

Copy link
Copy Markdown
Contributor Author

cc @spalger. This PR might cause a very small conflict in your theming PR. Note that for K7 we should not be loading the K6 theme files any longer (which is what this PR does)

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@snide

snide commented Jan 23, 2019

Copy link
Copy Markdown
Contributor Author

Tests are related to the snake case file names and needing to update the PNGs for the functional tests. Will fix both in the morning.

@cchaos cchaos left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So the biggest changes that we will see will be due to the fact that the base font size has changed from 14px to 16px. Most of the time this should be fine, especially where EUI is being used. However, you can see some pretty drastic size changes in screens like these:


There are some instances where the smaller font size is probably preferred, like in the title bars of dashboard panels. So we might want to comb through a bit to see where we might want to reduce the font size.

Comment thread src/ui/public/assets/fonts/readme.md Outdated
Comment thread src/ui/public/assets/fonts/readme.md Outdated
@snide
snide requested a review from a team as a code owner January 24, 2019 04:54
@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@snide
snide requested a review from a team as a code owner January 24, 2019 06:35
@snide
snide requested a review from a team January 24, 2019 07:01
@snide

snide commented Jan 24, 2019

Copy link
Copy Markdown
Contributor Author

I spent a few hours doing cleanup of font issues. There are likely to be more, but i think the very obvious ones were attacked.

Couple notes for discussion tomorrow:

  • I unified markdown across kibana to use the previously canvas layer that @ryankeairns wrote. It uses ems. We might want to think of a way to make this more flexible for vis usage. Mostly though I didn't want us having so many markdown files doing weird versions of the same thing.
  • I upgraded EUI to get @cchaos' font stack updates.
  • I had to touch bootstrap to apply a font size to their condensed form.
  • I commented out the variable font usage for inter UI. It has some issues I'll discuss with the designers tomorrow.
  • We have some font weight issues around our bold weighting. Likely an easy change in EUI, but it's noticeable in the form labels with smaller fonts.

@snide

snide commented Jan 24, 2019

Copy link
Copy Markdown
Contributor Author

I added some notes to the top for teams who are getting pinged due to snapshot / minor font changes.

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@snide

snide commented Jan 31, 2019

Copy link
Copy Markdown
Contributor Author

@spalger I merged up now that nav is in. Unfortunately this PR still breaks in the same places :(

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine

Copy link
Copy Markdown
Contributor

💚 Build Succeeded

@snide

snide commented Feb 1, 2019

Copy link
Copy Markdown
Contributor Author

Yay @spalger. Merging. @w33ble I made the changes you requested.

@snide
snide merged commit 037fcf4 into elastic:master Feb 1, 2019
@snide
snide deleted the k6/is_dead branch February 1, 2019 05:00
@gchaps

gchaps commented Feb 11, 2019

Copy link
Copy Markdown
Contributor

@snide Could you please add this change to the 7.0 Breaking Changes doc?

patrykkopycinski pushed a commit to patrykkopycinski/kibana that referenced this pull request May 6, 2026
…astic#29152)

Adds inter ui as the default font for Kibana. Removes the K6 theming. Kibana now uses EUI default.
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.

7 participants