-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Improved rendering/handling of relation members #11701
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
base: develop
Are you sure you want to change the base?
Conversation
* more space for member label by putting buttons next to role input * remove whitespace between connected members * show connected/looping members by icons * add grab handles to indicate drag&drop functionality
todo: translatable
1. select relation 2. shift-select any number of features to be added as additional members to the relation 3. right-click on any of the features and select the "merge" operation (or press the hotkey `C`) 4. the features are added as new members to the relation (either automagically sorted into a reasonable order, trying to keep members connected, or otherwise appended at the end) with an empty role
offered when a selection contains: * one relation * one or more features that are a member of the relation
similar to how this is already implemented in the raw membership editor
1ec5
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The redesign of the member/membership list adds important functionality, but I’m concerned that non-expert users will experience some difficulty learning how it works on their own. Additionally, consider some additions to the Relations section of the help documentation.
So far, this PR focuses on the sidebar, but the map is still underutilized. The map could highlight the endpoints of each gap and any locations where the member ways’ directions collide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took me a bunch of squinting at your examples before realizing that the pattern consists of down arrows on one side and up arrows on the other side. I thought this was just a grippy pattern, similar to icon-grab.svg.
| .each(function(d) { | ||
| if (d.connections.prev && d.connections.next || d.connections.loops) { | ||
| d3_select(this).call(svgIcon('#iD-icon-grab-connects-both')); | ||
| } else if (d.connections.prev) { | ||
| d3_select(this).call(svgIcon('#iD-icon-grab-connects-prev')); | ||
| } else if (d.connections.next) { | ||
| d3_select(this).call(svgIcon('#iD-icon-grab-connects-next')); | ||
| } else { | ||
| d3_select(this).call(svgIcon('#iD-icon-grab')); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Showing the connections is a good idea, but I really don’t find the backwards/forwards distinction intuitive at all. It isn’t clear enough to me that the grippies are supposed to connect, probably because the labels get in the way.
In case the grippy patterns don’t work out, here are some alternative ideas:
- Keep the gap between each member. Insert the arrows in the gap. Increase the gap and insert some obvious indicator wherever there are discontinuities.
- Keep the arrow indicators where they are, but remove the handles. Make it more like JOSM’s “model railroad” schematic, with lines running continuously up and down the section, but ideally without the gaps that JOSM has.
- Combine all the handles for the connected members into a single handle. Dragging that handle moves all the members together, sort of like in a jigsaw puzzle.
- Give each row a jigsaw puzzle piece shape. The pieces fit together when they’re connected on the map. They don’t fit if there’s a gap or the roles conflict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do make the rows look like jigsaw puzzle pieces, they could make a subtle but satisfying click when you snap together pieces that had been disconnected: #5821. 😏
| const relationCount = entityIDs.filter(entityID => | ||
| graph.entity(entityID).type === 'relation') | ||
| .length; | ||
| if (relationCount !== 1) return 'not_eligible'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Merge action remains enabled even when the Remove Members action is enabled, so the context menu starts out with ➖ followed by ➕. Do we need to have both actions enabled at the same time? Users shouldn’t need to add redundant memberships very often, but when they do have to, they can still add it the old-fashioned way with the way selected.
| paths_intersect: These features can't be merged because the resulting path would intersect itself. | ||
| too_many_vertices: These features can't be merged because the resulting path would have too many points. | ||
| members_remove: | ||
| title: Remove Members |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove Members doesn’t look like an operation that undoes what Merge does. How about Unmerge? Or maybe make it look like a version of the Detach operation.
| } | ||
| } | ||
|
|
||
| graph = actionDeleteMembers(relation.id, memberIndices)(graph); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a feature is a member of the relation multiple times, this operation completely removes all its memberships from the relation. This makes it less useful for dealing with incorrect redundant memberships, though it would make more sense if the operation were called something like Detach.
| wrapEnter | ||
| .append('button') | ||
| .classed('members-download', true) | ||
| .classed('form-field-button', true) | ||
| .attr('title', t('icons.download')) | ||
| .call(svgIcon('#iD-icon-load')) | ||
| .on('click', downloadMembers); | ||
|
|
||
| items.selectAll('button.members-download') | ||
| .classed('hide', d => { | ||
| const graph = context.graph(); | ||
| return d.relation.members.every(m => graph.hasEntity(m.id)); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regardless of the handle design, it’s still quite tedious to fix the order in a relation that’s all jumbled up. A button to sort the whole relation would give the user a good starting point to refine further: #7498 (comment).
| .each(function(d) { | ||
| if (d.connections.prev && d.connections.next || d.connections.loops) { | ||
| d3_select(this).call(svgIcon('#iD-icon-grab-connects-both')); | ||
| } else if (d.connections.prev) { | ||
| d3_select(this).call(svgIcon('#iD-icon-grab-connects-prev')); | ||
| } else if (d.connections.next) { | ||
| d3_select(this).call(svgIcon('#iD-icon-grab-connects-next')); | ||
| } else { | ||
| d3_select(this).call(svgIcon('#iD-icon-grab')); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do make the rows look like jigsaw puzzle pieces, they could make a subtle but satisfying click when you snap together pieces that had been disconnected: #5821. 😏
Several improvements related to how one can interact with relation members:
(For the screenshots below: on the left is new UI, right is the previous one.)
forward/backwardroles in more complex relationstodo: