Saved queries design cleanup #44191
Saved queries design cleanup #44191
Conversation
💔 Build Failed |
andreadelrio
left a comment
andreadelrio
left a comment
There was a problem hiding this comment.
Thanks for all these improvements @cchaos
Note to the other reviewers: We've added the checkmark icon next to the selected query to help differentiate it from the rest of the list items (especially when the other items get hovered). This is the result:
cchaos
added
Team:Visualizations
|
Pinging @elastic/kibana-app |
cchaos
added
release_note:skip
snide
left a comment
snide
left a comment
There was a problem hiding this comment.
Need a focus state for the items in the list. Currently only the trash icon shows. Rest of the code looks good.
|
@snide Yeah, that's an issue with EuiListGroupItem, already created a ticket: elastic/eui#2266 |
snide
left a comment
snide
left a comment
There was a problem hiding this comment.
OK. Focus state isn't the end of the world (we can follow up) and it's a nice change. Looks much better. Nice work @cchaos and @andreadelrio
💔 Build Failed |
cchaos
force-pushed
the
saved-queries-design-cleanup
branch
from
335ac30 to
ce6be7f
Compare
💔 Build Failed |
|
Jenkins, test this , pretty please 💛 |
|
Jenkins, test this |
There was a problem hiding this comment.
The UI looks great but there's a privilege related problem with the cleanup:
When in read-only mode, the delete icon shouldn't appear. It is at the moment, so a read-only user can delete a saved query when then shouldn't be able to. We use showWriteOperations here to determine if a user has privileges to delete, modify and save. This is no longer being used.
This is failing the security functional tests (X-pack Chrome Functional tests/Group5 and Group3)(read-only mode) in Discover, Dashboard and Visualize. I checked those tests locally and see the failures.
|
Thanks @TinaHeiligers I believe I fixed those in ce6be7f Can you make sure you have the latest? |
|
@cchaos I'll check again with the latest. |
|
retest |
TinaHeiligers
left a comment
TinaHeiligers
left a comment
There was a problem hiding this comment.
UI looks great!
The security tests for saved queries also pass locally now.
LGTM
💔 Build Failed |
|
retest |
cchaos
force-pushed
the
saved-queries-design-cleanup
branch
from
ce6be7f to
cf5d21d
Compare
|
Retest |
💚 Build Succeeded |
6 participants
Follow up from #39140
Design pass on the Saved Queries Management popover
With help from @andreadelrio we've tidied up the look of the popover. Here are the main points:
1. Using BEM naming scheme for classes
Example:
.saved-query-list-wrapper-->kbnSavedQueryManagement__listWrapperAlso, using the correct file structure for SASS files: using
_index.scssfor manifest and imports two files adjacent to the components they target.2. Better empty state
Changed the text to be smaller and using EuiPopoverFooters for better division of content vs buttons.
3. Using EuiListGroup and EuiListGroupItem for the actual list
This gives us better selected states, delete icon on hover only and truncation.
4. Fixes the height/max-height
The popover was setting a fixed height at all times causing it to be tall needlessly. This PR uses max-height, instead.
And the overflow still works
Even in IE
5. Adds a checkmark to selected items
... to make it more apparent that it's "selected".
Note: The above screenshots are a bit out of date, instead of replacing them all the following is what selected looks like.
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support[ ] Documentation was added for features that require explanation or tutorialsFor maintainers
[ ] This was checked for breaking API changes and was labeled appropriately[ ] This includes a feature addition or change that requires a release note and was labeled appropriately