Reconcile CNT vs. vanilla LTN landing pages #206
Conversation
michaelkirk
changed the title
| @@ -61,7 +61,7 @@ export type Mode = | |||
| export let map: Writable<Map | null> = writable(null); | |||
|
|
|||
| // The exact key in local storage | |||
| export let projectName: Writable<string> = writable(""); | |||
| export let currentProjectKey: Writable<string> = writable(""); | |||
There was a problem hiding this comment.
This was just a rename to make it more obvious that it's not only the project name.
There was a problem hiding this comment.
This is very elegant and works great! I love that it functions as a full SPA, instead of being split over two pages. My tests of it all went fine, both for CNT and the other mode. Switching projects and creating new ones all works.
When clicking "Choose project", it was initially unusual to not have the map zoom not reset, but I think I prefer it that way, having gotten used to it.
I vote we defer the question of migrating the non-CNT path to using new-style keys. We do want to do it, but similar to #169, there may be a few more breaking changes in the next few weeks. A clean break and proper savefile versioning in a few weeks would be great, but would just slow us down in the meantime.
| @@ -52,6 +52,8 @@ | |||
| return; | |||
| } | |||
|
|
|||
| // REIVEW: can we change to be like this to align with ltn_cnt project keys? | |||
There was a problem hiding this comment.
This would affect newly created projects. But in getProjectList(), we still have the old format. We could look for both there, or decide to make a breaking change and switch to ltn/. I think we should do that breaking cutover for consistency, but maybe not just yet? Related to #195 also.
| route_pt_a, | ||
| route_pt_b, | ||
| } from "../stores"; | ||
| import { Backend } from "../wasm"; | ||
|
|
||
| // Returns whether the project name is already taken, otherwise the project is created. |
There was a problem hiding this comment.
I see, this is only used for CNT right now, but we could cut over to it in NewProjectMode too.
There was a problem hiding this comment.
Even though it could work to use this for new projects now, I'm leaning towards holding off on adopting the "new key structure" for vanilla LTN until we are also ready to migrate the old keys to the new format. I think it will just keep things a little simpler to think about. I'll write up my understanding in an issue.
Update: here it is #207
TODO
- [ ] Can we consolidate load/save logic
- [ ] Revisit "is_cnt" logic (showing context)
- [ ] remain rooted in cnt.html after selecting
DONE
- [x] Resize panels appropriately
- [x] Right panel shows Scotland map
- [x] fix popups
- [x] Choosing scotland zooms to bounds={[-8.943, 54.631, -0.901, 59.489]}
- [x] Persist selected focus across reloads (by virtue of it being in the URL!)
- [x] choose CNT by URL, not by widget
- [x] Only show CNT projects in CNT
- [x] Maybe move selection widget to left panel, since it's only
editable on that first page.
- [x] Selection widget
- [x] Left panel lists items
- [x] Remove cnt.html (UNDONE!)
It also includes 'ltn_' or 'ltn_cnt/' and also the `studyAreaName` in the case of CNT.
michaelkirk
force-pushed
the
mkirk/reconcile-landing
branch
from
0bd2d50 to
7b008ee
Compare
I confess that wasn't an intentional change. In the back of my mind I knew that something had changed, but in my testing, I'd never zoomed very far in, so it wasn't too disorienting to come back to most of the scotland map. Of course, immediately after merging, I went to edit a neighbourhood (so was quite zoomed in), and it felt pretty disorienting to be back at the national level and only see a few square kilometers of Scotland. Personally, I think it would be better if we zoomed back out to the initial Scotland bbox. Can you speak on your preference for the new behavior I introduced? Maybe there's some middle ground that keeps what you like. |
|
I was picturing a sudden jump in the map, which felt weird, but you're right...the use case in "choose project" demands a lower zoom. Filed #209 |
FXIES #205 (part of #104)
This ended up not being a huge change (if you hide whitespace diff). In fact it got much smaller over time as I continued to work on it. But wow, each of these lines was hard fought.
Recall my first attempt was at #104 (comment) which unified the
ltnvsltn_cntlanding pages, and introduced a dropdown to switch "app focus" between a "global" view and a "scotland" specific view.After talking it through with @dabreegster, it seems like that's not actually likely to reflect how cnt people will use the app.
Where I landed, conceptually, is very similar from a datamodel perspective! We maintain an "appFocus" but now it's simply hardcoded by the root component based on the URL you access, e.g.:
App(element, {appFocus: "cnt"})I could imagine a more dynamic approach, but for now we only have two flavors, so I think it's fine to keep separate pages for
ltn/index.htmlandltn/cnt.htmlAdditionally, you are no longer "redirected" after creating/picking your ltn_cnt project. You stay rooted at
ltn/cnt.htmlthe entire time.And as a bonus, the CNT landing page is now rendered in our typical "layout", which I think is nice from a consistency standpoint.
url-root.mov.mp4
I think there is further followup we could do. In particular, from #104
Unifying some of these things will let us further de-dupe our implementations that currently have some copy/pasta, but I think they can be done in a subsequent PR.
Special request: I've tested this a bit, but probably not thoroughly enough yet. Can you give it a thorough hammering? I've learned I need to be more careful when mucking with the persistence layer, since we already have some production users.