-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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] Replace doc viewer table with EuiInMemoryTable #102149
Conversation
src/plugins/discover/public/application/components/doc_viewer/doc_viewer.scss
Show resolved
Hide resolved
@elasticmachine merge upstream |
merge conflict between base and head |
I've checked it out and played with it and so far it works pretty well, I'm adding some questions about design when using the new table FYI @andreadelrio @ryankeairns So this is what I would call the "breaking" change, the built in responsiveness of the EUI component, adding @shaunmcgough @timroes for awareness (As we know, old component, hasn't changed significantly in a long while) @dmitriynj |
Regarding the responsive view. I am not sure this will be really benefitial for users, since it's wasting A LOT of whitespace. I'd suggest we disable this for now (as far as I remember there is a property to disable responsive behavior on the table), and evaluate that separate how we might want to design that screen in a better responsive way. |
…ve-ui-doc-viewer # Conflicts: # src/plugins/discover/public/application/components/table/table.test.tsx # src/plugins/discover/public/application/components/table/table.tsx
I just checked this on smaller screens as well. IMO, the table loses a lot of usability unless For medium sized devices I think we should remove truncation from the field name and let the badge wrap. It would look like this: |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@wylieconlon also can't reproduce the issues you're reported, can you pls check again? many thx! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, my layout issues are resolved now. I am also seeing the bug that @kertal is reporting about the lack of actions in standalone mode.
fieldName={fieldName} | ||
fieldMapping={fieldMapping} | ||
flattenedField={flattenedField} | ||
onFilter={onFilter!} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type casting for onFilter
callback is safe here, due to ACTIONS_COLUMN
will not be used when onFilter
is undefined. See howtableColumns
memoized array defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Design changes LGTM 🎉
</EuiToolTip> | ||
</EuiFlexItem> | ||
|
||
{isMultiField && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice addition!
}, | ||
})); | ||
const component = mountComponent(props); | ||
|
||
const categoryKeywordRow = findTestSubject(component, 'tableDocViewRow-category.keyword'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the implementation has changed a bit, should be testing for the lack of multifieldBadge
here, same as in the previous test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! Added.
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / X-Pack Accessibility Tests.x-pack/test/accessibility/apps/spaces·ts.Kibana spaces page meets a11y validations a11y test for space selection pageStandard Out
Stack Trace
Metrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: cc @dmitriynj |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving, since the display of (empty)
is unrelated to this PR, it's also on master. Tested locally in Chrome, Firefox, Safari. Works as expected. Great work! 🥳
Note about |
…2149) * [Discover] replace legacy table with euiInMemoryTable * [Discover] update styles, add badge * fix font in badge and adjust line height * add tooltip * [Discover] update unit tests, return actions column to left side * [Discover] update field name test snapshot * [Discover] update wording * [Discover] handle pagination, return formatting value styles * [Discover] fix failing stylelint error * [Discover] return responsive prop, update classes * [Discover] update test and meet formatting rules * improve table view on medium * remove extra file * [Discover] fix unit test * [Discover] align top vertically field name and action cells, disable table responsive design * [Discover] adjust styles for cross browser compatibility * [Discover] remove pagination optimize styles, update test * [Discover] fix eslint * [Discover] clean up styles * [Discover] fix single doc view * [Discover] add check lack of multifieldBadge Co-authored-by: Andrea Del Rio <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
…103678) * [Discover] replace legacy table with euiInMemoryTable * [Discover] update styles, add badge * fix font in badge and adjust line height * add tooltip * [Discover] update unit tests, return actions column to left side * [Discover] update field name test snapshot * [Discover] update wording * [Discover] handle pagination, return formatting value styles * [Discover] fix failing stylelint error * [Discover] return responsive prop, update classes * [Discover] update test and meet formatting rules * improve table view on medium * remove extra file * [Discover] fix unit test * [Discover] align top vertically field name and action cells, disable table responsive design * [Discover] adjust styles for cross browser compatibility * [Discover] remove pagination optimize styles, update test * [Discover] fix eslint * [Discover] clean up styles * [Discover] fix single doc view * [Discover] add check lack of multifieldBadge Co-authored-by: Andrea Del Rio <[email protected]> Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Andrea Del Rio <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
Closes #86757
Summary
Large screen devices
Medium screen devices
Mobile devices
Checklist