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] Improve Doc viewer #107869

Merged
merged 10 commits into from
Aug 17, 2021
Merged

[Discover] Improve Doc viewer #107869

merged 10 commits into from
Aug 17, 2021

Conversation

dimaanj
Copy link
Contributor

@dimaanj dimaanj commented Aug 9, 2021

Summary

This PR fixes issues, missed in previous one:

  • Adds auto layout, which changes column width on resize, see video below;
  • Fixes row height, like it was historically, see screenshot here;
  • Removes mobile headers with display: none; rules, see picture below;

Redundant DOM nodes

DF743477-EA94-4D6F-9C14-7DEF279B7029_1_105_c

Auto layout

doc_viewer.mp4

Checklist

@dimaanj dimaanj added Feature:Discover Discover Application Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.15.0 labels Aug 9, 2021
@dimaanj dimaanj requested review from kertal, timroes, majagrubic and a team August 9, 2021 08:43
@dimaanj dimaanj self-assigned this Aug 9, 2021
@dimaanj dimaanj requested a review from a team as a code owner August 9, 2021 08:43
@dimaanj dimaanj added this to PRs in progress in Discover via automation Aug 9, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@dimaanj
Copy link
Contributor Author

dimaanj commented Aug 9, 2021

@elasticmachine merge upstream

@dimaanj dimaanj mentioned this pull request Aug 9, 2021
Merged
3 tasks
kertal
kertal reviewed Aug 10, 2021
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.

Theres's one thing about setting the table layout to auto, when e.g. a single filename is very long, the whole fields column consumes much space. so I wonder if there's e.g. a way to limit the column width to a max of 50%? cc @ryankeairns
Discover_-_Elastic_und_Discover_-_Elastic

@dimaanj
Copy link
Contributor Author

dimaanj commented Aug 11, 2021

@elasticmachine merge upstream

@kertal
Copy link
Member

kertal commented Aug 11, 2021

Theres's one thing about setting the table layout to auto, when e.g. a single filename is very long, the whole fields column consumes much space. so I wonder if there's e.g. a way to limit the column width to a max of 50%? cc @ryankeairns
Discover_-_Elastic_und_Discover_-_Elastic

dear @andreadelrio & @ryankeairns what's your opinion here, should we auto-layout the table like it was before we switched to the EUI component? many thx!

@ryankeairns
Copy link
Contributor

@kertal my apologies. I started poking around at this yesterday and got sidetracked. This is a tough one. While I see your point, I'm not certain how well an arbitrary max will solve the issue. There are so many unknowns at play with what people's data will look like and this feels like we're potentially making a 'quick fix' based on sample data.

I'm inclined to not add anything and let it flow naturally. Is there further context I'm missing here?

@kertal
Copy link
Member

kertal commented Aug 12, 2021

@kertal my apologies. I started poking around at this yesterday and got sidetracked. This is a tough one. While I see ywitour point, I'm not certain how well an arbitrary max will solve the issue. There are so many unknowns at play with what people's data will look like and this feels like we're potentially making a 'quick fix' based on sample data.

I'm inclined to not add anything and let it flow naturally. Is there further context I'm missing here?

no worries, I'm fine with that just keeping it auto , so it's like it worked before. Just wanted to raise this, to ask for opinion as I'm also not 100% convinced. thx!

@kertal
Copy link
Member

kertal commented Aug 12, 2021

@elasticmachine merge upstream

kertal
kertal reviewed Aug 16, 2021
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 👍 will test it tomorrow morning

src/plugins/discover/public/application/components/table/table.tsx Show resolved Hide resolved
src/plugins/discover/public/application/components/table/table.tsx Outdated Show resolved Hide resolved
@dimaanj dimaanj requested a review from kertal August 17, 2021 07:15
kertal
kertal approved these changes Aug 17, 2021
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 in Chrome, Firefox, Safari, works as expected, thx for those adaptations 👍

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
discover 430 429 -1

Async chunks

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

id before after diff
discover 554.9KB 555.5KB +600.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
discover 93.8KB 93.0KB -826.0B

History

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

cc @dmitriynj

@dimaanj dimaanj added the auto-backport Deprecated: Automatically backport this PR after it's merged label Aug 17, 2021
@dimaanj dimaanj merged commit e258f46 into elastic:master Aug 17, 2021
Discover automation moved this from PRs in progress to Done Aug 17, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Aug 17, 2021
@dimaanj @kibanamachine
[Discover] Improve Doc viewer (elastic#107869)
0c0da42 
* [Discover] fix doc-vewer

* [Discover] remove redundant stuff

* [Discover] remove redundant i18n

* [Discover] remove unused translation

* [Discover] fix by comments

* [Discover] clean up remaining things

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine kibanamachine mentioned this pull request Aug 17, 2021
Merged
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
✅ 7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Aug 17, 2021
@kibanamachine @dimaanj
[Discover] Improve Doc viewer (#107869) (#108847)
b5eb1fa 
* [Discover] fix doc-vewer

* [Discover] remove redundant stuff

* [Discover] remove redundant i18n

* [Discover] remove unused translation

* [Discover] fix by comments

* [Discover] clean up remaining things

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Dmitry Tomashevich <39378793+Dmitriynj@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryankeairns ryankeairns ryankeairns approved these changes

@kertal kertal kertal approved these changes

@timroes timroes Awaiting requested review from timroes

@majagrubic majagrubic Awaiting requested review from majagrubic

Assignees

@dimaanj dimaanj

Labels
auto-backport Deprecated: Automatically backport this PR after it's merged Feature:Discover Discover Application release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.15.0 v8.0.0
Projects
No open projects
Discover
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@dimaanj @elasticmachine @kertal @ryankeairns @kibanamachine