Skip to content
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

Proper file management for scheme sketch #539

Merged
merged 18 commits into from
Nov 7, 2024
Merged

Proper file management for scheme sketch #539

merged 18 commits into from
Nov 7, 2024

Conversation

dabreegster
Copy link
Contributor

@dabreegster dabreegster commented Nov 6, 2024

Previously, SS only handled one local storage file per (authority, schema) pair. This introduces proper multi-file management. The user flow is now spread out across more pages:

  1. choose an area
  2. create, import, or load a file
  3. sketch

To handle local storage issues that could potentially be triggered on any page (#488), this PR also introduces a simpler approach of detecting the quota problem, then redirecting to a new dedicated page for helping the user resolve the problem.

This PR tries to match Figma designs as much as possible. The list below is the MVP for this PR, and we can tackle smaller followups in #536:

  • Adapt existing tests to the new flow

files, and then finally sketch.

Optionally, move the old local storage item to the new naming scheme.
Not totally sure this worked.

Download a file from the manage files page too

Reorganize sidebar buttons. Instructions and app version aren't helpful. Add a title.

Plumb through schema everywhere. On the file page, list files from all schemas.

Simplify top of the sidebar -- breadcrumbs, less area info

Tune sidebar style after integration with new scheme management

Consolidate code to load/import a geojson file

Remove a confusing column on the manage files page
- name: Run type-checker
run: npm run check

- name: Run Playwright tests
run: npm test -- --reporter=html
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reporter bit adds an artifact the action logs. It's a downloadable HTML page that's nicer to view, but it's still clunky -- will look into this ctrf reporter project later

if (schema == "pipeline") {
filename += "_pipeline";

// TODO Trust the authority set in there, or recalculate it?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's this and another oddity of extra browse fields being plumbed through. I'll address this in the next PR on SS/SB integration.

@@ -47,7 +46,6 @@
<Alpha />
<div style="border-bottom: 1px solid #b1b4b6">
<LoggedIn />
<p>App version: {appVersion()}</p>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting closer to Figma. This was just developer info; it's inferrable from the URL anyway


test("a v1 file with a pipeline hint redirects to pipeline mode", async ({
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behavior is removed; people can create a new file and add scheme from file instead

@dabreegster dabreegster marked this pull request as ready for review November 7, 2024 15:06
@dabreegster dabreegster requested a review from Pete-Y-CS November 7, 2024 15:08
@dabreegster
Copy link
Contributor Author

OK, this is done! Tests have been overhauled -- they're now much more isolated from each other, with most starting a new file per test case. Behaviorally, the most unusual thing is how importing a file works. Unless it looks like a pipeline file, it'll get detected as the v1 schema. If a user needs to change these, they need to create an empty pipeline file and do "add schemes from file" instead. In practice, this won't be a problem. If we make a new schema for ATF5, we can revisit how the user specifies the type of file they create to make this more clear.

I've combed through the code changes carefully and made sure tests all make sense, and tried this for real against all the random sketch files in different areas I've got. I'm going to merge now, async review is welcome, and we have #536 to track followups.

@dabreegster dabreegster merged commit 6049efc into main Nov 7, 2024
2 checks passed
@dabreegster dabreegster deleted the manage_files branch November 7, 2024 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant