Datagrid toolbar #4
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 5 commits
November 8, 2021 10:53
And lower left padding
Removed all the unnecessary manual additions of `className="euiDataGrid__controlBtn”`
cee-chen
reviewed
Nov 8, 2021
cee-chen
reviewed
Nov 8, 2021
| anchorPosition="downRight" | ||
| panelPaddingSize="s" | ||
| panelClassName="euiDataGrid__controlPopover" | ||
| panelClassName="euiDataGrid__stylePopoverPanel" |
Owner
There was a problem hiding this comment.
This still needs the generic .euiDataGrid__controlPopover class, as that CSS has transform/z-index stuff specific to all toolbar popovers
Suggested change
| panelClassName="euiDataGrid__stylePopoverPanel" | |
| panelClassName="euiDataGrid__controlPopover euiDataGrid__stylePopoverPanel" |
Author
There was a problem hiding this comment.
That transform stuff is actually only needed on popovers whose contents contains the drag-and-drop functionality like the sort and column selector. This new popover is just a simple form.
Owner
There was a problem hiding this comment.
Ohhh that's super good to know! Now I feel like I should have named that CSS class better. I'll fix that after this merges
added 2 commits
November 8, 2021 14:50
cee-chen
reviewed
Nov 8, 2021
Comment on lines
+51
to
+62
| useEffect(() => { | ||
| // When data grid is full screen, we add a class to the body to remove the extra scrollbar and stay above any fixed headers | ||
| if (isFullScreen && typeof document !== 'undefined') { | ||
| document.body.classList.add(GRID_IS_FULLSCREEN_CLASSNAME); | ||
|
|
||
| return () => { | ||
| if (typeof document !== 'undefined') { | ||
| document.body.classList.remove(GRID_IS_FULLSCREEN_CLASSNAME); | ||
| } | ||
| }; | ||
| } | ||
| }, [isFullScreen]); |
Owner
There was a problem hiding this comment.
Super awesome fix, thanks! Do you think this needs a changelog entry or probably not?
Author
There was a problem hiding this comment.
Wouldn't hurt to give it a callout
Owner
There was a problem hiding this comment.
Nice, I added a changelog entry and a closes commit (which I think should auto close on merge to main?)
Author
There was a problem hiding this comment.
I honestly don't know if a commit message will. But might as well add it to your PR summary as well.
cee-chen
reviewed
Nov 8, 2021
Comment on lines
+68
to
+71
| const buttonLabel = useEuiI18n( | ||
| 'euiStyleSelector.buttonText', | ||
| 'Display options' | ||
| ); |
cee-chen
reviewed
Nov 8, 2021
| } | ||
| } | ||
|
|
||
| .euiDataGrid__controlBtn { |
Owner
There was a problem hiding this comment.
Fantastic, thanks for cleaning this up!
cee-chen
pushed a commit
that referenced
this pull request
Nov 10, 2021
* [Setup] Move toolbar CSS to own file * [Cleanup] DRY out repeated specific CSS class - naming was incorrect in the case of the style selector popover * Reorder toolbar per Caroline's mocks - Columns & Sort on left, with additionalControls to the right of those (+ update prop types to indicate that) - Style (TBD naming) and Fullscreen to the right * Convert display buttons to icons only + tweak grid styles icon to match Caroline's mock + popover location * Convert density UI to a compressed EuiFormRow w/ new text/order per mocks - refactor densityOptions to const outside hook * Update test snapshots * Add changelog entry * [PR feedback] Revert location of additionalControls + fix props description * [PR feedback] CSS feedback * [PR feature request] Change toolbar icon based on grid density * [PR feedback][extra] Fix missing sort control on styling example NB: sorting functionality won't actually work/isn't hooked up, but this should at least allow the show/hide UI toggle to work as expected * updoot snaps * Fix changelog * [misc] Reorganize EuiDataGridToolbar utils to bottom of file - makes it easier for devs to immediately jump to the main component/fn, rather than have to scroll past helper/minutae details - if it gets complex enough, also worth pulling out to a separate file * Rename CSS toolbar class to left/right to match API - Per GH discussion display/data is ambiguous and naming this left/right is likely more helpful * Add API support for left/right positioning of `additionalControls` * Update additionalControls documentation + example * PR feedback & cleanup from Caroline (#4) * Update some props comments and fix popover’s responsiveness * [Design] Remove a few extraneous style overrides to control buttons And lower left padding * Fixed add/remove of fullscreen body class * Added tooltips around icon buttons with long delay * Updated examples and snippets with “real world” examples Removed all the unnecessary manual additions of `className="euiDataGrid__controlBtn”` * More responsive fixes * Change class name * Update CSS class to be more specific - It isn't necessary for popovers without drag & drop, name becomes verbose but it's hopefully worth it for clearer usage * closes elastic#5092 - add changelog for fullscreen bugfix in Caroline's work * [PR feedback] Changelog * Update snapshots * PR feedback * Remove unused CSS class - Class was removed in a previous commit by Caroline Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@constancecchen
Here's a quick PR (I've detailed the changes in the commits). Mostly just some design tweaks, adding tooltips, and updating the examples to showcase "Real world" examples.
Let me know if you have any questions.