-
Notifications
You must be signed in to change notification settings - Fork 17
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
base: next
Are you sure you want to change the base?
Conversation
|
||
// Unused. Kept here to maintain compatibility. |
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.
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.
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.
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?
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.
SGTM, but maintainers would have the final call.
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.
How do I get a maintainer to look at this PR? :D
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.
@shilman ?
Description
Fixes #1
serialize
method to return a stringdeserialize
method to take a stringdeserialize
function to take the knob component, so that itsoptions
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.