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: use labelBuilder for equality in list knob #729

Merged
merged 4 commits into from Jun 8, 2023

Conversation

nuesslerm
Copy link
Contributor

@nuesslerm nuesslerm commented Jun 7, 2023

List of issues which are fixed by the PR

#730

Description
list knobs with labelBuilder or non-string values can fail to update

Steps To Reproduce

  1. Go to knobs_example
  2. Change add icon via list knob
  3. icon doesn't change

Screenshots

Screen.Recording.2023-06-07.at.21.15.00.mov

Checklist

  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making].
  • All existing and new tests are passing.

If you need help, consider asking for advice on Discord.

@docs-page
Copy link

docs-page bot commented Jun 7, 2023

To view this pull requests documentation preview, visit the following URL:

docs.widgetbook.io/~729

Documentation is deployed and generated using docs.page.

@YoussefRaafatNasry YoussefRaafatNasry changed the title fix(list_knob): fix valueFromQueryGroup fix: use labelBuilder for equality in list knob Jun 8, 2023
Copy link
Member

@YoussefRaafatNasry YoussefRaafatNasry left a comment

Choose a reason for hiding this comment

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

Great fix, @nuesslerm! Keep it up 🎉

I did some changes, to be able to merge it to main:

  1. Excluding irrelevant changes in knbos_example.
  2. Adding CHANGELOG entry for the fix.

Consider these in your future PRs, to get a faster merge.

@YoussefRaafatNasry YoussefRaafatNasry enabled auto-merge (squash) June 8, 2023 09:09
@YoussefRaafatNasry YoussefRaafatNasry self-assigned this Jun 8, 2023
@nuesslerm
Copy link
Contributor Author

@YoussefRaafatNasry thanks will do. 🙏
is this PR good to merge with a new version release? I see the deploy action is failing, but I can't see details on why that is. 🤔

@YoussefRaafatNasry
Copy link
Member

@nuesslerm No worries about the failing job.
We will merge the PR and it will be released in out rc.2 version very soon.

@nuesslerm
Copy link
Contributor Author

thanks! 🙏

@jenshor jenshor disabled auto-merge June 8, 2023 13:21
@jenshor jenshor merged commit 1b3237b into widgetbook:main Jun 8, 2023
4 of 5 checks passed
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.

None yet

5 participants