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

[Discover] Switch to Inter font for the grid and doc viewer #166373

Closed
wants to merge 1 commit into from

Conversation

jughosta
Copy link
Contributor

Part of Discover redesign v1.

This PR changes the font to Inter for the grid and doc viewer. Numeric values will still be well aligned.

Screenshot 2023-09-13 at 17 40 53 Screenshot 2023-09-13 at 17 41 04

@jughosta jughosta added release_note:enhancement Feature:Discover Discover Application backport:skip This commit does not require backporting Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. labels Sep 13, 2023
@jughosta jughosta self-assigned this Sep 13, 2023
@jughosta jughosta requested a review from a team September 13, 2023 16:08
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
discover 555.2KB 555.2KB -92.0B
unifiedDocViewer 56.3KB 56.3KB -46.0B
total -138.0B

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @jughosta

@jughosta jughosta marked this pull request as ready for review September 13, 2023 16:53
@jughosta jughosta requested a review from a team as a code owner September 13, 2023 16:53
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

I like the visual style 👍 However, there's something to consider for textual content, than needs mono spaced fonts to be displayed correctly, here's an example, where it doesn't work with Inter (It's our own painless lab smiley). left: Inter, right: The previous font
Bildschirmfoto 2023-09-18 um 08 21 00

While I don't think ingesting ASCII art is a primary use case for our users, there are certainly use cases where rendering this way matters.

E.g. we have Elasticsearch error message using an ascii error to help the user find the error in the previous line, which shows the code / query. If we format this with Inter, the arrow might point to the wrong part of the code / query. Here's a bad example, that high likely works also with Inter, but just to explain what I mean:

WDYT @andreadelrio @MichaelMarcialis

@MichaelMarcialis
Copy link
Contributor

I like the visual style 👍 However, there's something to consider for textual content, than needs mono spaced fonts to be displayed correctly, here's an example, where it doesn't work with Inter (It's our own painless lab smiley).

While I don't think ingesting ASCII art is a primary use case for our users, there are certainly use cases where rendering this way matters.

E.g. we have Elasticsearch error message using an ascii error to help the user find the error in the previous line, which shows the code / query. If we format this with Inter, the arrow might point to the wrong part of the code / query.

If we all feel that the aesthetic and readability improvements that come with switching to Inter outweigh our need to support rendering ASCII in the Discover table, then that only leaves the concern about rendering ASCII arrows in error messaging. Given that the error message appear to exist outside of the Discover table context, I'd personally be fine if we continue to use Roboto Mono there (while keeping Inter for the Discover table contents). We should also likely remove or replace that ASCII smiley face from the Painless lab if that's something that is commonly viewed in Discover.

Alternatively, if we feel that the aesthetic and readability improvements don't outweigh our need to render ASCII, then we should talk to the original working group again to discuss alternatives (as font selection seemed to be a key aspect of the design approval). Thoughts?

@davismcphee
Copy link
Contributor

I share @kertal's concerns about migrating to Inter (although I have to admit in the general case it's more pleasing to the eyes for sure).

If we all feel that the aesthetic and readability improvements that come with switching to Inter outweigh our need to support rendering ASCII in the Discover table, then that only leaves the concern about rendering ASCII arrows in error messaging. Given that the error message appear to exist outside of the Discover table context, I'd personally be fine if we continue to use Roboto Mono there (while keeping Inter for the Discover table contents).

I think (correct me if I'm wrong) @kertal's point here is less to do with our error message callouts and more so implying that our users may rely on similar functionality when viewing their own data in the grid. For example, a user may have an error_log index containing error messages with similar ASCII formatting (not uncommon for application logs).

Alternatively, if we feel that the aesthetic and readability improvements don't outweigh our need to render ASCII, then we should talk to the original working group again to discuss alternatives (as font selection seemed to be a key aspect of the design approval). Thoughts?

I'm not sure how common the ASCII formatting case actually is, but I imagine it's a relatively niche case in the grand scheme of things, and it may not make sense to hold off on design improvements over such a use case. But at some point I feel like we had discussed the ability to toggle between Inter and a monospace font (using Inter as the default). Maybe that approach would give us the best of both worlds, although at the risk of introducing yet another config for the grid. What are your thoughts on something like this?

@andreadelrio
Copy link
Contributor

I'm not sure how common the ASCII formatting case actually is, but I imagine it's a relatively niche case in the grand scheme of things, and it may not make sense to hold off on design improvements over such a use case. But at some point I feel like we had discussed the ability to toggle between Inter and a monospace font (using Inter as the default). Maybe that approach would give us the best of both worlds, although at the risk of introducing yet another config for the grid. What are your thoughts on something like this?

I would either add this as an option (users choose Inter or Roboto) or hold off on this change altogether. We know for a fact that the industry standard is using mono-space fonts to display logs so I'm hesitant to go against that. Add to that, we don't have any evidence that users want this change. In general, I think our users are less about what looks pretty and more about what is more efficient (show me as much data possible at once, minimize the number of clicks I have to do to perform an action, optimize space) and my hunch is that they're used to this type of font to parse their logs. We could potentially do user research to enquire about this but we are currently focused on a much larger research initiative (Discover Vision) and this would fall under low priority compared to that.

@jughosta
Copy link
Contributor Author

Okay, closing this PoC for now.

@jughosta jughosta closed this Sep 20, 2023
@kertal
Copy link
Member

kertal commented Sep 20, 2023

I think (correct me if I'm wrong) @kertal's point here is less to do with our error message callouts and more so implying that our users may rely on similar functionality when viewing their own data in the grid. For example, a user may have an error_log index containing error messages with similar ASCII formatting (not uncommon for application logs).

yes, exactly, it's about error messages like this ingested into Elasticsearch and when rendered they are displayed in a wrong way, I have another example from my todays cloud dashboard edits:

Bildschirmfoto 2023-09-20 um 09 08 12

I almost forgot, we used to render the table content with the default font, but switched to a monotype font a while ago, because it was requested: #131513

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Discover Discover Application release_note:enhancement Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants