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][EuiDataGrid] Add document navigation to flyout #94439

Merged
merged 15 commits into from
Mar 23, 2021
Merged

[Discover][EuiDataGrid] Add document navigation to flyout #94439

merged 15 commits into from
Mar 23, 2021

Conversation

kertal
Copy link
Member

@kertal kertal commented Mar 11, 2021 •
edited
Loading

Summary

This PR is adding document navigation to the document flyout of the EUIDataGrid implementation, so you can navigate to the previous and next document of the list.

Bildschirmfoto 2021-03-11 um 16 09 57

expand-document-navigation.mp4

Checklist

Delete any items that are not applicable to this PR.

@kertal kertal added the Feature:Discover Discover Application label Mar 11, 2021
@kertal kertal self-assigned this Mar 11, 2021
@kertal kertal changed the title [Discover][EuiDataGrid] Add document navigation [Discover][EuiDataGrid] Add document navigation to flyout Mar 11, 2021
@kertal
Copy link
Member Author

kertal commented Mar 15, 2021

dear @elastic/kibana-design , could you have a look to move it out of PoC land? many thx!

@ryankeairns
Copy link
Contributor

ryankeairns commented Mar 15, 2021 •
edited
Loading

Based on our testing, I believe this will be a welcome feature. Nice work!
(Also, it sets the stage for your other concept with the checkboxes 😉 )

Here are my quick takes after having run this locally... mainly, the pagination felt a bit too close to the 'X' (close) button.
Screen Shot 2021-03-15 at 9 25 38 AM

Placement of pagination

There is not as much available real estate, but moving the controls to the next line down feels like an appropriate grouping (everything in this row being secondary navigation) and increases space between the pagination/next and flyout/close buttons.

Note: to achieve this right-aligned layout, the flex item wrapper will need to grow and be justified to the end. I also decreased the gutter size on the parent to small.
Screen Shot 2021-03-15 at 9 35 53 AM

Mobile considerations (future)

The flyout could use a pass for better responsive layout styles. This situation (i.e. the row getting truncated) existed prior to this PR and the addition of pagination makes it more obvious. This can be carved out into a separate effort, if necessary.

Screen Shot 2021-03-15 at 9 20 48 AM

@kertal
Copy link
Member Author

kertal commented Mar 15, 2021

@ryankeairns yes, x (close) and pagination are overlapping, so this needs to be improved, and it looks nice in your mockup, I've got 1 argument to discuss here: grouping

When header and paginate are about in the same row, you could read them like "Expanded document 3 of 8", for me this is a more bit logic grouping that when it's on the second row ... but well on the other side you could also read it like

"Single document | Surrounding documents | Previous document (Position of document) Next document"

So while writing this I came to the conclusion, should be fine also in the second row 👍

@ryankeairns
Copy link
Contributor

I hear you. Let's try it on in the 2nd row and see how it fits :) Easy to relocate, if needed.

@kertal kertal mentioned this pull request Mar 17, 2021
Closed
@kertal
Copy link
Member Author

kertal commented Mar 18, 2021

@ryankeairns i've adapted the layout, could you have a look? Removed View for lower resolutions, anyway we might think of how to enable more user actions than this (in a later PR)

There seems to be an issue with pagination and responsive view?

Bildschirmfoto 2021-03-18 um 15 01 14

@ryankeairns
Copy link
Contributor

@kertal this is looking great.

I've added a few minor layout and responsive adjustments in this PR 👉 kertal#13

Merge at your leisure 🏖️

@kertal kertal marked this pull request as ready for review March 18, 2021 16:11
@kertal kertal requested a review from a team March 18, 2021 16:11
@kertal kertal requested a review from a team as a code owner March 18, 2021 16:11
@kertal kertal added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Mar 18, 2021
@elasticmachine
Copy link
Contributor

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

@kertal kertal added release_note:skip Skip the PR/issue when compiling release notes v7.13.0 v8.0.0 labels Mar 18, 2021
@kertal
Copy link
Member Author

kertal commented Mar 22, 2021

@elasticmachine merge upstream

@kertal kertal requested a review from majagrubic March 22, 2021 08:10
@kertal
Copy link
Member Author

kertal commented Mar 22, 2021

@ryankeairns QQ: document nav seems to be aligned differently in Chrome and Firefox, who's right?
Firefox:
Bildschirmfoto 2021-03-22 um 16 38 27

Chrome:
Bildschirmfoto 2021-03-22 um 16 38 51

@ryankeairns
Copy link
Contributor

@ryankeairns QQ: document nav seems to be aligned differently in Chrome and Firefox, who's right?
Firefox:
Bildschirmfoto 2021-03-22 um 16 38 27

Chrome:
Bildschirmfoto 2021-03-22 um 16 38 51

Oops, Firefrox is right. Might need to use flex-end instead of just end.

@shaunmcgough shaunmcgough mentioned this pull request Mar 22, 2021
Closed
41 tasks
@kertal
Copy link
Member Author

kertal commented Mar 23, 2021

@ryankeairns thx! now it works!

kertal added 2 commits March 23, 2021 09:22
@kertal
Fix SCSS
ea089e9
@kertal
Merge branch 'kertal-pr-2020-03-11-discover-data-grid-document-naviga…
669ce8a 
…tion' of github.com:kertal/kibana into kertal-pr-2020-03-11-discover-data-grid-document-navigation
@kibanamachine
Copy link
Contributor

💚 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 383.6KB 387.0KB +3.4KB

History

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

cc @kertal

majagrubic
majagrubic approved these changes Mar 23, 2021
View reviewed changes
Copy link
Contributor

@majagrubic majagrubic left a comment

Choose a reason for hiding this comment

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

Tested this in Chrome on Mac OSX, works as expected 👍

@kertal kertal merged commit 6a85841 into elastic:master Mar 23, 2021
@kertal kertal mentioned this pull request Mar 23, 2021
Merged
kertal added a commit to kertal/kibana that referenced this pull request Mar 23, 2021
@kertal @ryankeairns
[Discover][EuiDataGrid] Add document navigation to flyout (elastic#94439
616335c 
)

- allows the user to navigate to the previous and next document of the list in the document flyout

Co-authored-by: Ryan Keairns <[email protected]>
kertal added a commit that referenced this pull request Mar 23, 2021
@kertal @ryankeairns
[Discover][EuiDataGrid] Add document navigation to flyout (#94439) (#…
856c2f4 
…95185)

- allows the user to navigate to the previous and next document of the list in the document flyout

Co-authored-by: Ryan Keairns <[email protected]>

Co-authored-by: Ryan Keairns <[email protected]>
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

@majagrubic majagrubic majagrubic approved these changes

Assignees

@kertal kertal

Labels
Feature:Discover Discover Application release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.13.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kertal @ryankeairns @elasticmachine @kibanamachine @majagrubic