UI Update; Editing mode Update; Bulk-Update-Page; Manual-Update-Page#103
Conversation
This also moves some code into CenterlineTagActions that was used only there
Make it one wort so it fits with the others
Prominent UI to explain the current page
Streamline wording and make it more compact.
Less own color; makes it easier to work with many colors in the style Packages: update svelte-utils
We should not generate sidewalks for highway=trunk etc.
We only wand sidewalks generated when they are mapped explicitly here.
Simplify a189e6b5441fec1f5bb59b6b09012497238cf2e7
But keep the arrow since this is kind of our "selected line" state
Using localstorage
White on the buttons, not on the wrapper
And cleanup left over bulk actions components.
tordans
changed the title
|
@dabreegster this PR got a bit big and I wanted to reach out to hear how you would want to handle this. Like before, the Svelte code is something I can review well and which I optimized by hand. But the Rust code is generated and reviewed only by using/testing it in the app. The question is, how to get this ready for Speedwalk Summary of the changes: WayDetails & tag handling
Jumbotron & UI copy
New & reworked pages
Sidewalk features & UX
Legends & shared components
The app has its own legend helper now; not those from the utils repo. This was just easier for now… Map & basemap
Highway classification
Editing UX & validation
Shared state & UI polish
A few Screenshots but not of all new features… 
|
dabreegster
left a comment
dabreegster
left a comment
There was a problem hiding this comment.
Wow, this is an incredible 10 steps forward on the app! I'm a massive fan of the UX changes (separating the app usage into pure OSM auditing and more advanced network generation, the jumbotron, reorganizing bulk actions into one clean place, using indexed DB instead of local storage, etc). I've given everything a try locally and it all seems to be working great.
I'm happy to merge this now. Realistically I don't have any time to review this in more detail for at least the next month, and I trust what you've done. I gave a quick skim to all the code and had a few notes, but mostly just nitpicks. By and large everything is totally understandable and well-structured from what I can tell. The comments I left could be notes for me to go back in a month and tweak things a bit.
Seriously awesome, thank you so much for pushing this forward
| // the sidewalk generator. | ||
| if tags.is_any( | ||
| "highway", | ||
| vec![ |
There was a problem hiding this comment.
Refactor with is_severance, except the service road case?
| #[derive(Clone, Serialize)] | ||
| pub enum UserCmd { | ||
| SetTags(WayID, Vec<(String, String)>), | ||
| SetTags(WayID, Vec<String>, Vec<(String, String)>), |
There was a problem hiding this comment.
Would be useful to add a `/// Doc comment saying what these are; the Vec<(String, String)> was key/value pairs, and I'm guessing the new thing is keys to delete. Or switch to a struct for this case with named fields
| UserCmd::SetTags(way, replace) => { | ||
| UserCmd::SetTags(way, remove_keys, add_tags) => { | ||
| let cmds = self.change_way_tags.entry(way).or_insert_with(Vec::new); | ||
| // Clear old sidewalk tags first |
There was a problem hiding this comment.
Ah good, the responsibility for this moves to the frontend then
| import { bbox } from "svelte-utils/map"; | ||
|
|
||
| const DB_NAME = "speedwalk-overrides"; | ||
| const STORE_NAME = "regionOverrides"; |
There was a problem hiding this comment.
What does "region" mean in context here?
There was a problem hiding this comment.
Two bits of UX feedback here:
- It'd be cool if the two points were
draggable<Marker>s - A primary button to confirm instead of just a keybinding could be useful for discoverability (though I agree the keyboard is faster to do once you're used to it)
(PR a-b-street#103) Keep SetTags(WayID, Vec<String>, Vec<(String, String)>) with explicit fields. Add doc comment and inline comments (remove_keys, add_tags) so the two vecs are clearly defined.
(PR a-b-street#103) - Add SEVERANCE_HIGHWAY_TYPES in lib.rs as single source of truth for motorway through tertiary_link (no service). - Add is_severance_highway(tags) and is_road_without_sidewalks_implicit(tags); the latter is severance types OR highway=service for RoadWithoutSidewalksImplicit. - Way::is_severance() now uses is_severance_highway(); classify uses is_road_without_sidewalks_implicit() so service stays explicit and the rest reuse the same definition.
(PR #103) Keep SetTags(WayID, Vec<String>, Vec<(String, String)>) with explicit fields. Add doc comment and inline comments (remove_keys, add_tags) so the two vecs are clearly defined.
(PR #103) - Add SEVERANCE_HIGHWAY_TYPES in lib.rs as single source of truth for motorway through tertiary_link (no service). - Add is_severance_highway(tags) and is_road_without_sidewalks_implicit(tags); the latter is severance types OR highway=service for RoadWithoutSidewalksImplicit. - Way::is_severance() now uses is_severance_highway(); classify uses is_road_without_sidewalks_implicit() so service stays explicit and the rest reuse the same definition.
2 participants
See #103 (comment)