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.

Sign up for GitHub

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

Jump to bottom

[Discover] Monospace font in Document Explorer #131513

Merged
merged 8 commits into from
May 6, 2022
Merged

[Discover] Monospace font in Document Explorer #131513

merged 8 commits into from
May 6, 2022

Conversation

jughosta
Copy link
Contributor

@jughosta jughosta commented May 4, 2022 •
edited
Loading

Closes #130402

Summary

This PR changes the default font to Monospace for Document Explorer. It also changes font family and reduces font size in cells popover.

Screenshot 2022-05-05 at 13 49 47

Checklist

@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. v8.3.0 labels May 4, 2022
@jughosta jughosta self-assigned this May 4, 2022
@jughosta jughosta force-pushed the 130402-monospace-font branch from 00801f3 to 9681979 Compare May 4, 2022 12:43
@jughosta
Copy link
Contributor Author

jughosta commented May 4, 2022

@kertal Is it possible to define a width for timestamp column so it fits dates into 1 line? After changing the font family it started wrapping to the second line.

@kertal
Copy link
Member

kertal commented May 5, 2022

@jughosta yes, here you can change it:

kibana/src/plugins/discover/public/components/discover_grid/constants.ts

Line 22 in a7e0171

export const defaultTimeColumnWidth = 190;

@jughosta jughosta force-pushed the 130402-monospace-font branch 3 times, most recently from 905383a to 070df8b Compare May 5, 2022 09:02
@jughosta jughosta marked this pull request as ready for review May 5, 2022 09:03
@jughosta jughosta requested review from a team as code owners May 5, 2022 09:03
@elasticmachine
Copy link
Contributor

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

kertal
kertal reviewed May 5, 2022
View reviewed changes
src/plugins/discover/public/components/discover_grid/discover_grid.scss Outdated

.dscDiscoverGrid__cellPopoverValue {
font-family: $euiCodeFontFamily;
font-size: $euiFontSizeXS;
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest $euiFontSizeS, which is a bit larger. I think to make it larger is fine, since the popover is here to have a closer look, which is easier with a larger font
Bildschirmfoto 2022-05-05 um 11 48 23

kertal
kertal approved these changes May 5, 2022
View reviewed changes
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.

Code LGTM, tested locally with Chrome, Firefox, Safari, MacOS. Now Doc Exp looks a bit more like classic table, one reason less to go back. 👍 from engineering. The decision about the expanding popover? In @elastic/kibana-design we trust

@kertal
Copy link
Member

kertal commented May 5, 2022

The failing tests should be easy to fix, but I just noted that our JEST test look a bit ugly ... we should fix those:
Bildschirmfoto 2022-05-05 um 11 57 43

@jughosta jughosta force-pushed the 130402-monospace-font branch from 070df8b to 1121050 Compare May 5, 2022 11:43
@jughosta jughosta mentioned this pull request May 5, 2022
Closed
@jughosta
Copy link
Contributor Author

jughosta commented May 5, 2022 •
edited
Loading

The failing tests should be easy to fix, but I just noted that our JEST test look a bit ugly ... we should fix those:

@kertal Created a ticket for it #131621

ryankeairns
ryankeairns reviewed May 5, 2022
View reviewed changes
Copy link
Contributor

@ryankeairns ryankeairns left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looking good; noticed a couple of things:

  1. It looks like 200 may not be wide enough. I edited the date to a larger number and it wrapped.
  2. The row height switcher seems to be broken, but I see that it doesn't work aside from this PR. I'll open a separate issue for this (the 'Switch to default' is the issue).

Also, the font size change on the popover looks good. This way it is still just one step up from the cell font size (12 to 14px).

@kertal
Copy link
Member

kertal commented May 5, 2022

  1. The row height switcher seems to be broken, but I see that it doesn't work aside from this PR. I'll open a separate issue for this (the 'Switch to default' is the issue).

@ryankeairns are your referring to this issue ? elastic/eui#5593

@jughosta
Copy link
Contributor Author

jughosta commented May 5, 2022

It looks like 200 may not be wide enough. I edited the date to a larger number and it wrapped.

@ryankeairns Thanks for the review! What width would you recommend to set instead?

@ryankeairns
Copy link
Contributor

It looks like 200 may not be wide enough. I edited the date to a larger number and it wrapped.

@ryankeairns Thanks for the review! What width would you recommend to set instead?

I'm not certain which MMM DD is longest, but I tried a few and 210 worked consistently.

@jughosta
Copy link
Contributor Author

jughosta commented May 5, 2022

@ryankeairns Updated to 210, thanks!

@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 408.1KB 408.6KB +505.0B

History

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

cc @jughosta

@ryankeairns ryankeairns self-requested a review May 6, 2022 15:49
ryankeairns
ryankeairns approved these changes May 6, 2022
View reviewed changes
Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

Thank you for the change

@jughosta jughosta merged commit e55f8c8 into elastic:main May 6, 2022
@jughosta jughosta deleted the 130402-monospace-font branch May 6, 2022 16:21
jloleysens added a commit to jloleysens/kibana that referenced this pull request May 9, 2022
@jloleysens
Merge branch 'main' of github.com:elastic/kibana into reporting/use-c…
18f3bee 
…hromium-to-print-pdf-part-1

* 'main' of github.com:elastic/kibana: (59 commits)
  [Cloud Posture] Enabled findings group by feature (elastic#131780)
  [EBT] Fix `userId` generation (elastic#131701)
  [RAM] Add shareable rule tag filter (elastic#130710)
  Optimize package installation performance, phase 2 (elastic#131627)
  [Screenshotting] instrument for benchmark tests using new EventLogger class (elastic#130356)
  [Connector] Adding internal route for requesting ad-hoc ServiceNow access token (elastic#131171)
  [ci] bump kibana-buildkite-library (elastic#131754)
  [Synthetics] UI clean up (elastic#131598)
  [RsponseOps] Fix flaky rules list test (elastic#131567)
  [Cases] Add severity field to create case (elastic#131626)
  [Discover] Monospace font in Document Explorer (elastic#131513)
  Sessions tab improvements (elastic#131583)
  Add cloud icon "ess-icon" at the end of the config keys in "alerting" documentation (elastic#131735)
  [DOCS] Updates deprecation text for legacy APIs (elastic#131741)
  [ci] break out skip patterns so they can change without triggering CI (elastic#131726)
  Adjust search session management page font size (elastic#131291)
  [Unified search] Fix uptime css problem (elastic#131730)
  [Actionable Observability] Link to filtered rules page (elastic#131629)
  Add openAPI specifications for cases endpoint (elastic#131275)
  Display rule API key owner to users who can manage API keys (elastic#131662)
  ...

# Conflicts:
#	x-pack/plugins/screenshotting/server/formats/pdf/index.ts
#	x-pack/plugins/screenshotting/server/screenshots/observable.ts
kertal pushed a commit to kertal/kibana that referenced this pull request May 24, 2022
@jughosta @kertal
[Discover] Monospace font in Document Explorer (elastic#131513)
24ed1ec 
* [Discover] Monospace font in Document Explorer

* [Discover] Update code style

* [Discover] Update jest snapshot

* [Discover] Update jest snapshot

* [Discover] Increase the default width for time column

* [Discover] Reduce font size for cell popover

* [Discover] Update font size for cell popover and fix tests

* [Discover] Update width to 210
@kertal kertal mentioned this pull request Sep 20, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kertal kertal kertal approved these changes

@ryankeairns ryankeairns ryankeairns approved these changes

Assignees

@jughosta jughosta

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. v8.3.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Discover] monospace font in Document Explorer
5 participants
@jughosta @kertal @elasticmachine @ryankeairns @kibana-ci