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

Add style URL to the hash #62

Merged
merged 7 commits into from
Apr 19, 2023
Merged

Add style URL to the hash #62

merged 7 commits into from
Apr 19, 2023

Conversation

aparlato
Copy link
Contributor

Closes #48

If you add a style via url, that style will be added to the hash. Then on refresh, the style will be fetched again. If you add a new style via drag and drop, the url will be removed from the hash.

Copy link
Contributor

@ebrelsford ebrelsford left a comment

Choose a reason for hiding this comment

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

There seems to be a loop that gets triggered for me that we should take care of--presumably once the style loads the store is changing and triggering a reload of the style?

To reproduce:

  1. Load chartographer
  2. Enter a URL for a valid style
  3. Enter a URL for a different valid style

Expected output: the style at the URL provided in (3) is visualized

Actual output: the page alternates between the URLs in (2) and (3), and you can see the style URLs in the hash alternating as well

if (activeUrl) {
fetchStyle(activeUrl);
}
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only really wanted this to run once, so I changed App.svelte to set variables from the hash before mounting in since we don't need anything to render first for that which sets us up to use onMount more appropriately downstream here.

@aparlato
Copy link
Contributor Author

aparlato commented Apr 17, 2023

@ebrelsford good catch! Just updated.

[edit]: Found another issue to resolve first

@aparlato
Copy link
Contributor Author

Ok, now I think this should be good to go.

Copy link
Contributor

@ebrelsford ebrelsford left a comment

Choose a reason for hiding this comment

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

Looking great!

One last thing: I think the selectedTab parameter in the hash is getting overridden. That is, if I load a style URL, select the lines chart, and refresh the style is loaded but the fills chart is always shown.

I'm not sure if that was introduced here, if it wasn't let's open a follow-up issue for it.

@aparlato
Copy link
Contributor Author

aparlato commented Apr 19, 2023

I'm not sure if that was introduced here, if it wasn't let's open a follow-up issue for it.

Yea, that one's a bit deeper of an issue, but an intentional choice. We decided to always flip back to the fill page to replenish the background layer:

// On dropping in a style, switch to the fill tab to refresh background layer state
handleTabChange({ detail: { tab: 'fill' } });

This carries over to styles in the URL since they're treated as a new style and need a background layer.

[edit]: Opened issue here: #63

Copy link
Contributor

@ebrelsford ebrelsford left a comment

Choose a reason for hiding this comment

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

Got it, thanks!

@aparlato aparlato merged commit 1f2a9dd into main Apr 19, 2023
@aparlato aparlato deleted the hash-url branch April 19, 2023 18:50
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.

Add style URL as stateful URL
2 participants