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.

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

[UnifiedFieldList] Add a drag handle to field list items #171572

Closed
wants to merge 7 commits into from

Conversation

jughosta
Copy link
Contributor

@jughosta jughosta commented Nov 20, 2023

Summary

This PR adds a drag icon next to field list items on hover.

Preview

Nov-20-2023 19-21-26
Nov-20-2023 19-22-22

This reverts commit 1d8ae3b.

# Conflicts:
#	packages/kbn-unified-field-list/src/containers/unified_field_list_sidebar/field_list_sidebar.scss
@jughosta jughosta added the Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. label Nov 20, 2023
@jughosta jughosta self-assigned this Nov 20, 2023
@andreadelrio
Copy link
Contributor

Hi @jughosta ! Thanks for working on this. I've put together a comparison of the current state of this PR vs the mockups. Not entirely sure why but the icon doesn't seem to be centered, there appears to be more space on the right than on the left. I've included some specifications in regards to spacing between items. Also, with the inclusion of the grabOmnidirectional icon I think we should be able to remove the translateX defined in @mixin mixinDomDragDropHover.

Frame 2

@jughosta
Copy link
Contributor Author

jughosta commented Dec 1, 2023

Thanks, @andreadelrio! I will look into it.

@jughosta
Copy link
Contributor Author

jughosta commented Dec 4, 2023

Hi @andreadelrio,

I updated the styles, please have a look in Discover and Lens when you get a chance.

@andreadelrio
Copy link
Contributor

Hi @andreadelrio,

I updated the styles, please have a look in Discover and Lens when you get a chance.

Hi @jughosta thanks for the updates. This is looking much better. The Lens version doesn't feel as good to me as the Discover one. In particular, this bit (see video below). I will check with @MichaelMarcialis to see what he thinks.

Screen.Recording.2023-12-04.at.4.13.21.PM.mov

className="unifiedFieldListItemDragHandle"
css={css`
pointer-events: none;
flex-shrink: 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

@jughosta do you still need this line? I'm seeing this in the browser.

image

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
discover 707 709 +2
eventAnnotationListing 460 462 +2
lens 1147 1149 +2
logExplorer 644 646 +2
total +8

Async chunks

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

id before after diff
cloudSecurityPosture 443.9KB 444.0KB +144.0B
dataVisualizer 614.6KB 614.8KB +144.0B
discover 589.6KB 591.4KB +1.8KB
enterpriseSearch 2.7MB 2.7MB +144.0B
eventAnnotationListing 197.0KB 197.1KB +144.0B
graph 388.2KB 388.3KB +144.0B
indexManagement 585.2KB 585.3KB +144.0B
lens 1.4MB 1.4MB +1.7KB
maps 2.9MB 2.9MB +144.0B
ml 3.6MB 3.6MB +144.0B
presentationUtil 82.5KB 82.6KB +144.0B
securitySolution 12.9MB 12.9MB +144.0B
stackAlerts 203.0KB 203.2KB +144.0B
unifiedDocViewer 58.5KB 58.6KB +144.0B
unifiedSearch 216.0KB 216.1KB +144.0B
total +5.3KB

Page load bundle

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

id before after diff
osquery 51.3KB 51.4KB +144.0B

History

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

cc @jughosta

@MichaelMarcialis
Copy link
Contributor

MichaelMarcialis commented Dec 5, 2023

Hi @andreadelrio,
I updated the styles, please have a look in Discover and Lens when you get a chance.

Hi @jughosta thanks for the updates. This is looking much better. The Lens version doesn't feel as good to me as the Discover one. In particular, this bit (see video below). I will check with @MichaelMarcialis to see what he thinks.

Thanks for putting this together, @jughosta! It looks amazing. @andreadelrio and I discussed her concerns above in Slack. I'm personally not a fan of the fact that the appearance of the drag handle on hover/focus means the height of the field list item may increase due to additional line breaks appearing. This increased-height-on-hover possibility is an issue that will exist in both the Discover and Lens field list versions, and it's something I didn't anticipate during the design phase.

As a simple way to mitigate this issue, what if we changed it so that on hover/focus of a field list item we fade-out the token and fade-in the drag handle to replace the token (in the same position)? In doing so, we could also keep the old translate x-axis transition (where the field list item slides a few pixels to the right) to emphasize that it's draggable. That little bit of extra movement might be good, if the appearance of the drag handle is no longer pushing the text (given the above suggestion). Thoughts? I'd be happy to discuss further or make mockups if needed.


While I'm here, I also wanted to ask whether we should continue to style the field lists uniquely for Discover and Lens (as you have here), or if now would be a good time to proceed with a consistent field list styling for both applications. For example, is there a compelling reason for us not to remove the background color, shadows, border radii, and reduce the font size for the field list items in Lens (at least until a field list item is being dragged, in which case the background color, shadows, and border radii appear on the drag clone)? If there's not a compelling reason, I'd personally love to have the two field lists appear as similar as possible, if not altogether identical. Thoughts? CCing @ninoslavmiskovic, in case he has any concerns with such a recommendation.


Finally, I know there has been some friendly disagreement on this between @timductive and me in the Figma comments, but the original designs had each field list item as 32px tall with no margin in between siblings (versus the current 24px tall with 2px margin between). Doing so provided what I would consider to be minimal safe real estate with which users could grab and drag an item (though others tend to suggest even larger, ~38–40px minimum). While I understand there is a desire for high density in the field list, I think this needs to be balanced against usability. Would ya'll be open to increasing the height of each field list item to better match the designs?

The end result of this suggested change also isn't very dramatic on the field density topic. For example, in a 1440x1024 viewport, the current field height and margining shows 27.5 fields. Updating height and removing margins to match the designs shows 23 fields. I'm personally OK having 4.5 fields falling below the fold for the sake of a better experience.

@ninoslavmiskovic
Copy link
Contributor

++ On keep moving towards aligning Lens and Discover on field list UI. Like we did with features in the field list.

We should also "dry-run" the new design as a unified component that is embedded across solution.

Regarding the pixel height I also think it is important to test in other contexts.

@jughosta
Copy link
Contributor Author

Hi @MichaelMarcialis and @andreadelrio,

Sorry for the delay. I've implemented your suggestion for showing the drag icon in place of the field type icon. Here is a new PR for it: #173673

@jughosta
Copy link
Contributor Author

jughosta commented Dec 22, 2023

Closing in favor of #173673

@MichaelMarcialis For the changes in field item's height/margin would be great to have a separate Github issue. I also agree that it would be better to have the same styles on both Lens and Discover pages.

@jughosta jughosta closed this Dec 22, 2023
@jughosta jughosta deleted the 168856-field-list-drag-handle branch December 22, 2023 09:49
jughosta added a commit that referenced this pull request Jan 3, 2024
- Resolves #168856

## Summary

As per comment in
#171572 (comment)

> As a simple way to mitigate this issue, what if we changed it so that
on hover/focus of a field list item we fade-out the token and fade-in
the drag handle to replace the token (in the same position)? In doing
so, we could also keep the old translate x-axis transition (where the
field list item slides a few pixels to the right) to emphasize that it's
draggable. That little bit of extra movement might be good, if the
appearance of the drag handle is no longer pushing the text (given the
above suggestion).

![Dec-19-2023
18-13-21](https://github.com/elastic/kibana/assets/1415710/e02cb7d6-ce1a-4507-a8a6-3004f89d7225)


### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

---------

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants