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

Adds hover-highlighting for relations in the raw membership list #5429

Merged

Conversation

quincylvania
Copy link
Collaborator

Also:

  • Moves hover-highlighting behavior to its own function
  • Hover-highlighting now correctly highlights all members of the target relation

Completes task 1 of #2946. Continuation of #5402 and #5404.

screen shot 2018-10-24 at 10 11 10 pm

@bhousel Adding recursive highlighting for relations meant that the code snippet became too large to keep repeating everywhere, so I moved the highlighting logic to its own file/function. I'd like guidance on the naming and location of this file.

Moves hover-highlighting behavior to its own function
Hover-highlighting now correctly highlights all members of the target relation
@quincylvania quincylvania self-assigned this Oct 25, 2018
@bhousel
Copy link
Member

bhousel commented Oct 25, 2018

@bhousel Adding recursive highlighting for relations meant that the code snippet became too large to keep repeating everywhere, so I moved the highlighting logic to its own file/function. I'd like guidance on the naming and location of this file.

I'd put the highlighting code in https://github.com/openstreetmap/iD/blob/master/modules/util/util.js
or in its own file under util. (maybe reusing some of the code in util.js is there already).

The convention we follow is if the function is named utilSomethingSomething, the file should be util/something_something.js

👉 Also, relations are tricky and can be circular (a relation can contain itself or an ancestor as a child), so it's really best to avoid recursing. e.g. utilGetAllNodes uses a seen variable to avoid infinite looping: https://github.com/openstreetmap/iD/blob/master/modules/util/util.js#L44

Breaks out the code for getting a selector for entities and all their descendants into a generic function
Accounts for circular relations when recursively getting all relation member IDs
@quincylvania
Copy link
Collaborator Author

@bhousel Thanks for the tip on circular relations! I hadn't considered those were possible. I made some changes, let me know if you have further suggestions. Otherwise it's ready.

@bhousel
Copy link
Member

bhousel commented Oct 27, 2018

Looks great, thanks!

@bhousel bhousel merged commit 0570956 into openstreetmap:master Oct 27, 2018
@quincylvania quincylvania deleted the relation-membership-highlighting branch October 27, 2018 19:12
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.

None yet

2 participants