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

Core: Add globals URL param and remove from sessionStorage #15056

Merged
merged 8 commits into from
May 27, 2021
Merged

Conversation

ghengeveld
Copy link
Member

Issue: #11604

What I did

Added globals URL query param that tracks any globals that have a value that deviates from their initial/default value.
Removed session storage for globals.

How to test

  • Is this testable with Jest or Chromatic screenshots? no
  • Does this need a new example in the kitchen sink apps? no
  • Does this need an update to the documentation? yes, todo

If your answer is yes to any of these, please make sure to include it in your PR.

@nx-cloud
Copy link

nx-cloud bot commented May 26, 2021

Nx Cloud Report

CI ran the following commands for commit b0919fa. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=prepare --all --parallel --max-parallel=15

Sent with 💌 from NxCloud.

@nx-cloud
Copy link

nx-cloud bot commented May 26, 2021

Nx Cloud Report

We didn't find any information for the current pull request with the commit 7dc469c.
Please make sure you set the \ NX_BRANCH\ environment variable in your CI pipeline .

Check the Getting started section to configure the app.


Sent with 💌 from NxCloud.

Copy link
Member

@shilman shilman 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 w/ one suggestion! 💯

lib/client-api/src/types.ts Outdated Show resolved Hide resolved
Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

Couple questions

lib/client-api/src/story_store.test.ts Outdated Show resolved Hide resolved
lib/api/src/modules/url.ts Outdated Show resolved Hide resolved
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

💯

@shilman shilman changed the title Core: Add globals URL param and remove globals from sessionStorage Core: Add globals URL param and remove from sessionStorage May 27, 2021
@shilman shilman merged commit 5ea8cdd into next May 27, 2021
@shilman shilman deleted the url-globals branch May 27, 2021 12:03
@nickofthyme
Copy link

@ghengeveld @shilman is there a way to hide or prevent global params from syncing to the url? I initially thought this was a good idea but after building my own addon, this locks-in the global variable for all stories.

Say I have a story that has a background color as a global parameter. Say the parameter default is only set in the preview.js file to #FFFFFF for story1. This will read the color from the default parameters from preview.js and update the url. Now if I navigate to story2 that has a story-level parameter override with a default of #000000, the color is still set to #FFFFFF because of the query param.

I DO think the hierarchy is correct and the url param value should override any other defined value. But syncing the global values to the URL creates issues. Looking at the old knobs addon, the knob values where never written to the url but you could still copy the current knob state via the copy button, which would then provide a stateful url that includes the necessary query params.

I think the capture state method used with the knobs would be a better approach in the case of the globals. Is there a reason this was done this way and would you consider changing this?

@shilman
Copy link
Member

shilman commented Aug 3, 2021

cc @MichaelArestad ☝️

@ghengeveld
Copy link
Member Author

I think the capture state method used with the knobs would be a better approach in the case of the globals. Is there a reason this was done this way and would you consider changing this?

The rationale is that we wanted to make it super easy and obvious to share stories and their state with others. Syncing the URL makes it easy to just copy the URL from the address bar, regardless of how they got there. A separate Copy button would require feature discovery. We did consider a "share" button in the toolbar (not in the addon panel like Knobs has, because args and globals are a core feature), but decided against it for now. If URL syncing continues to be problematic we might reconsider. I'm pretty sure at that point we'll design a more elaborate "share" feature that gives you more than just a URL.

I think a short-term improvement would be to enable addon authors to opt-out their globals from URL syncing. Some globals aren't worth syncing with the URL, such as the measure addon state.

@nickofthyme
Copy link

I think a short-term improvement would be to enable addon authors to opt-out their globals from URL syncing. Some globals aren't worth syncing with the URL, such as the measure addon state.

Yes this would be greatly appreciated

@ghengeveld
Copy link
Member Author

#15756

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants