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

Add highlight to multi-select sidebar on hover of entity #7607

Conversation

jgscherber
Copy link
Contributor

This is to resolve #2949 and is a WIP

This is my first time working on this project (and first time working in a large JS codebase) so please don't hold back on style/design criticisms. I want to thank you for the Architecture readme that you have. It really helped with initially finding my way around.

Here's a demo of the highlight
highlight_demo

I ran into a couple issues while working on it

  • After adding the hoverModeSelect callback, performing a click-and-drag results in iD dropping out of "select" mode. Any ideas what's causing that?
  • The discussion in With multiple features selected, indicate which one is mouse-hovered in the sidebar multiselect list  #2949 says the resolution to this issue should also have the sidebar scroll into view the element in the list that's being highlighted - how can I check if an element is visible in the viewport?
  • This change adds a class .hover to perform the styling which duplicates the pseudo-class :hover -- is it better for the :hover pseudo-class be removed and only styled using the .hover class

@quincylvania
Copy link
Collaborator

@jgscherber Thanks for working on this! I'll take a look and give you feedback this week.

@@ -113,6 +113,24 @@ export function uiSidebar(context) {
.append('div')
.attr('class', 'inspector-hidden inspector-wrap fr');

var hoverModeSelect = function hoverModeSelect(datum) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to name this twice, you can pick either style.

return node.id === datum.id;
});

if (elements.size() > 0){
Copy link
Collaborator

Choose a reason for hiding this comment

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

This works fine but there's a .empty() function that's even easier.

@quincylvania
Copy link
Collaborator

@jgscherber Looks pretty good! We could merge as-is but I'll give you a chance to take another look if you want. Let me know if you have any questions.

After adding the hoverModeSelect callback, performing a click-and-drag results in iD dropping out of "select" mode. Any ideas what's causing that?

This was a bug with other changes I'd been making in the develop branch. If you remerge this should be fixed.

The discussion in #2949 says the resolution to this issue should also have the sidebar scroll into view the element in the list that's being highlighted - how can I check if an element is visible in the viewport?

I think we might actually live without this. I feel like it'd be disorienting more often than not. But to answer your question you can compare the list item element's position to scrollTop of the inspector.

This change adds a class .hover to perform the styling which duplicates the pseudo-class :hover -- is it better for the :hover pseudo-class be removed and only styled using the .hover class

Nah, there's no harm in two separate classes. I had to move all the :hover declarations to media queries anyway for #7432.

@jgscherber
Copy link
Contributor Author

I'm planning on changing your two suggestions. I'll fix my rebase also

@jgscherber jgscherber closed this May 23, 2020
@jgscherber jgscherber deleted the 2949-highlight-selection-list branch May 23, 2020 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

With multiple features selected, indicate which one is mouse-hovered in the sidebar multiselect list
2 participants