-
-
Notifications
You must be signed in to change notification settings - Fork 1
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
Load saved CNT projects #213
Conversation
let gj = JSON.parse(contents); | ||
// Is this a CNT project or a regular one? | ||
let key = ""; | ||
if (gj.study_area_name && gj.study_area_name.startsWith("LAD_")) { |
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.
This is... brittle and gross. I think part of #207 should also consider adding in appFocus or similar to the GJ savefile itself.
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.
I commented in #207 (I completely agree).
@@ -47,12 +48,16 @@ export async function createNewProject( | |||
|
|||
export async function loadFromLocalStorage(key: string) { | |||
currentProjectKey.set(key); | |||
let isCnt = key.startsWith("ltn_cnt/"); | |||
// TODO Should we also change the URL? |
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.
Should we rewrite history and set index.html or cnt.html?
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.
I think we should!
So if you're at ltn/index.html
and load a "cnt" project, it should "feel like" you're being redirected to ltn/cnt.html
.
We probably literally could 302 redirect them, but I don't know if that does anything for us except slow down their browsing experience a bit.
One of the things I'm trying to do to maintain sanity is to say that anything cnt happens under the ltn/cnt/
path. It's kind of an artificial rule, but since this "CNT mode" is a global-state thing, I think it might be helpful for our debugging sanity and mental model when that is visibly reflected in an obvious thing like the browsers current URL hierarchy.
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.
I'll get to this tomorrow; I'll merge but keep #195 open.
I like the cnt/
subdirectory vs cnt.html
, do we want to go with that too? Should be a pretty easy change in vite config and making a new cnt/index.html.
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's not challenging I don't think, just late / open PRs weigh on my mind a bit)
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.
I like the cnt/ subdirectory vs cnt.html, do we want to go with that too?
Sure!
TBH in my mind they are semantically equivalent, except that the .html
communicates that your website has been online since the Clinton administration.
dc7631a
to
f9a94b9
Compare
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.
LGTM - if you can easily add the url update/redirect, I'd think you should before merging.
If it's challenging, well... I guess this is still a useful improvement as is and we should merge.
let gj = JSON.parse(contents); | ||
// Is this a CNT project or a regular one? | ||
let key = ""; | ||
if (gj.study_area_name && gj.study_area_name.startsWith("LAD_")) { |
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.
I commented in #207 (I completely agree).
@@ -47,12 +48,16 @@ export async function createNewProject( | |||
|
|||
export async function loadFromLocalStorage(key: string) { | |||
currentProjectKey.set(key); | |||
let isCnt = key.startsWith("ltn_cnt/"); | |||
// TODO Should we also change the URL? |
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.
I think we should!
So if you're at ltn/index.html
and load a "cnt" project, it should "feel like" you're being redirected to ltn/cnt.html
.
We probably literally could 302 redirect them, but I don't know if that does anything for us except slow down their browsing experience a bit.
One of the things I'm trying to do to maintain sanity is to say that anything cnt happens under the ltn/cnt/
path. It's kind of an artificial rule, but since this "CNT mode" is a global-state thing, I think it might be helpful for our debugging sanity and mental model when that is visibly reflected in an obvious thing like the browsers current URL hierarchy.
Fixes #195
Diffbased on #211, please review that first.Now both the global and CNT "choose project" pages have a tool to load a saved GJ file. From either of those pages, you can load any kind of file, and the app will switch to global or CNT mode accordingly... except for changing the URL.