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

fix(addon-knobs): Knob values not properly deserialized from the URL #38

Open
wants to merge 8 commits into
base: next
Choose a base branch
from

Conversation

mctrafik
Copy link

Description

Fixes #1

  • Changes types for serialize method to return a string
  • Changes the type of deserialize method to take a string
  • Adds an optional parameter to deserialize function to take the knob component, so that its options can be matched against the serialized value.

NOTE: In this PR I primarily focused on the 'Select' knob as that's the one mentioned in the bug. I tried to fix the other ones, too, but didn't focus too deeply on them.

Tested

Added unit tests for serialization/deserialization
Manually verified that copying a link and pasting it populates complex values onload.


// Unused. Kept here to maintain compatibility.
Copy link
Contributor

@Silic0nS0ldier Silic0nS0ldier Sep 13, 2021

Choose a reason for hiding this comment

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

If this is part of the documented public API surface and no longer does anything, then a release including this PR would be a breaking change (changed behaviour). In which case it shouldn't stick around as it'll act as a source of confusion. Same goes for serializers below.

Copy link
Author

Choose a reason for hiding this comment

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

That's a good point. This isn't documented anywhere. What do you think about just removing these constants and document that they were removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM, but maintainers would have the final call.

Copy link
Author

Choose a reason for hiding this comment

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

How do I get a maintainer to look at this PR? :D

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy knob state URL looses value type
2 participants