Flesh out the web editor, remove drag-and-drop #258
Conversation
| this.originalTags = tags; | ||
|
|
||
| // Remove these immediately. If they're rendered invisibly, they mess up indices. |
There was a problem hiding this comment.
Still temporary. Seeing the center line where direction changes would be quite helpful visually.
| (lane) => lane.type != "separator" | ||
| ); | ||
|
|
||
| // All of the state is immutable, except for road. The source of truth for |
There was a problem hiding this comment.
This is different -- and simpler -- than before, when the source of truth was the sortable cards in the DOM. Anytime we make a mutation, we just re-render all of the cards from scratch. This is cheap enough.
There was a problem hiding this comment.
The simplicity gained by this approach is a huge win in my experience.
| }; | ||
|
|
||
| document.getElementById("new-driving").onclick = () => { | ||
| this.road.lanes.push({ |
There was a problem hiding this comment.
Always appending to the end is clunky. You had suggested having + buttons in between each pair of lanes, and presumbly a way to click the type and change it. I like that idea now that I type it out; I'll try that in a followup
Benchmark for bf8158eClick to view benchmark
|
|
This looks as easy to user as streetmix.net, if not more so. And it's simpler which is good (and more relevant to OSM). Transport focused OSM editing functionality here we come? |
|
Sorry I haven't been following development here that closely, so feel free to ignore me if I'm off base. I felt pretty good about the UI we iterated on for the abstreet lane editor, where you could drag-and-drop lanes to re-order them, but deleting was a click-button action. Maybe that's a reasonable route to work towards for this web implementation: dragon.mov.mp4 |
Good feedback, thanks! I think the A/B Street behavior is what we want to build towards here. This JS editor died off for a few months, and I want to get the ball rolling again by attempting #240 (comment). Drag-and-drop will come back, but maybe not with sortable.js. And having a separate button for deletion (instead of dragging a lane <-> a trash can) sounds like simpler UX too. |
web_editor/js/cards.js
Outdated
| node.innerHTML += `<div align="center">` + directionIcon(lane) + `</div>`; | ||
| node.innerHTML += `<div align="center">` + width(lane) + `</div>`; | ||
| node.innerHTML = `<div align="center">${typeIcon(lane)}</div>`; | ||
| node.innerHTML += `<div align="center">${directionIcon(lane)}</div>`; |
There was a problem hiding this comment.
Micro optimisation point: the rule of thumb is that hitting the DOM is an order of magnitude slower than anything pure JS (the reason virtual-dom libraries make sense).
So innerHTML += should be avoided as a matter of habit.
There was a problem hiding this comment.
Good to know. Fixed. I still wish there was a cheap + idiomatic way to turn a quick snippet of HTML into an element. Building up a fragment with template literals is very nice.
There was a problem hiding this comment.
Assigning to innerHTML is fine, it's just the append that is problematic, as described in this answer: https://stackoverflow.com/a/23339072
I think the template literals approach is a good style, just commit it to the DOM in one go :)
There was a problem hiding this comment.
Ahhhh thank you, I understand now. Serializing to HTML and then parsing everything out again sounds awful!
I'll keep things as all objects for now, because the next step will need to make the direction arrow clickable. AFAICT, there's not a nice way to mix template literals and attaching callbacks, without assigning an ID and then looking it up, or making one callback / listener at the parent level
There was a problem hiding this comment.
Using listeners on some parent can work well, especially using data- attributes instead of ids to communicate data.
web_editor/js/cards.js
Outdated
| function iconObj(name) { | ||
| var obj = document.createElement("img"); | ||
| obj.src = `assets/${name}.svg`; | ||
| obj.className = "clickable"; |
There was a problem hiding this comment.
We should use <button> when the semantics fit. It comes with a whole host of behaviours (like keyboard focus and activation, hover/active state etc.) and accessibility stuff. Even before we are paying attention to keyboard support or accessibility, it is a better baseline, imo.
This should be a button with an img inside it, and we can use CSS to remove the border, fix padding/margin etc.
There was a problem hiding this comment.
Great point! Fixed
web_editor/js/cards.js
Outdated
| app.render(); | ||
| }; | ||
| } else { | ||
| // TODO Show greyed out |
There was a problem hiding this comment.
If the "buttons" were <button>s, we could use the disabled attr, which comes with default styles and appropriate behaviours. E.g. onclick will never be triggered.
| (lane) => lane.type != "separator" | ||
| ); | ||
|
|
||
| // All of the state is immutable, except for road. The source of truth for |
There was a problem hiding this comment.
The simplicity gained by this approach is a huge win in my experience.
|
PTAL, I switched over to always building up DOM elements, and using buttons. |
Benchmark for a1f16c4Click to view benchmark
|
Co-authored-by: Ben Ritter <[email protected]>
Benchmark for 36b719aClick to view benchmark
|
screencast.mp4
Remove drag-and-drop for now, since it complicates the code, we were using a library I'm not sure is the right choice, and the UX of dragging a trash can to a lane vs a lane to a trash can is a bit unclear to me. Instead, just have some buttons inline to each card, and add new lanes to the end always.