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

Controls: Fix number control update when using useArgs hook #17247

Merged
merged 1 commit into from
Jan 17, 2022

Conversation

aritrakoley
Copy link
Contributor

@aritrakoley aritrakoley commented Jan 15, 2022

Issue: #15924

Number control does not update when using useArgs hook

What I did

Added a useEffect hook to Number.tsx so that each time a new value is received, the value visible to the user is also changed.

useEffect(() => {
    const newInputValue = typeof value === 'number' ? value : '';
    if (inputValue !== newInputValue) {
      setInputValue(value);
    }
  }, [value]);

How to test

The repo mentioned in the original issue has the code required to test this.
Keeping link here for convenience.
Repo to reproduce: https://github.com/zygisau/use-args-number-type-control-bug-repro

  • Is this testable with Jest or Chromatic screenshots? I'm not sure
  • Does this need a new example in the kitchen sink apps? No
  • Does this need an update to the documentation? No

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 Jan 15, 2022

☁️ Nx Cloud Report

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

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@nx-cloud
Copy link

nx-cloud bot commented Jan 15, 2022

☁️ Nx Cloud Report

We didn't find any information for the current pull request with the commit 4e6dfb4.
You might need to set the 'NX_BRANCH' environment variable in your CI pipeline.

Check the Nx Cloud Github Integration documentation for more information.


Sent with 💌 from NxCloud.

@patrick-mcdougle
Copy link
Contributor

This looks like a good fix to me.

@aritrakoley
Copy link
Contributor Author

I see some failure messages above and I'm wondering if there's anything else I'm supposed to do for this to be merged smoothly.
Screenshot from 2022-01-15 21-30-00

@shilman shilman changed the title Fix Issue #15924 (Number control does not update when using useArgs h… Controls: Fix number control update when using useArgs hook Jan 16, 2022
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.

Code LGTM.

@shilman -- do you think we should create an E2E for this scenario (args updated via external method should affect controls) as it seems pretty easy for us to break this in the future? I'm not quite sure how involved such an E2E test would be though.

@shilman
Copy link
Member

shilman commented Jan 17, 2022

@tmeasday not so easy in its current state. let's discuss how to make it easier as part of the canvas project. merging.

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.

None yet

4 participants