Skip to content
This repository has been archived by the owner on Jul 8, 2024. It is now read-only.

Fix: DOS-237 - Added props.onchange to the callback dependency of numeric inputs and added onKeyDown property so we can bind to it #58

Merged
merged 5 commits into from
Dec 21, 2023

Conversation

jsumiguin-causalens
Copy link
Contributor

@jsumiguin-causalens jsumiguin-causalens commented Dec 18, 2023

Motivation and Context

Currently, if the props.onChange has any changes on the parent component(i.e. a state variable on parent that changes for some reason) the function on props.onChange becomes outdated and only gets updated if the props.value changes.

Also, currently NumericInput does not expose the onKeyDown event so we can add our own keydown events to it.

Implementation Description

  • Added props.onChange to the list of dependencies of onChange callback.
  • Added test to test the props.onchange item
  • Added onKeyDown to the list of properties for NumericInput
  • Added tests for the newly added prop

Any new dependencies Introduced

N/A

How Has This Been Tested?

This has been tested locally and by unit tests.

PR Checklist:

  • I have implemented all requirements? (see JIRA, project documentation).
  • I am not affecting someone else's work, If I am, they are included as a reviewer.
  • I have added relevant tests (unit, integration or regression).
  • I have added comments to all the bits that are hard to follow.
  • I have added/updated Documentation.
  • I have updated the appropriate changelog with a line for my changes.

Screenshots (if appropriate):

Copy link
Collaborator

@krzysztof-causalens krzysztof-causalens left a comment

Choose a reason for hiding this comment

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

Looks good, let's also add a test case as you said, you can create a simple wrapper component in-line within the test and check the behaviour

Copy link
Contributor

@patricia-causalens patricia-causalens left a comment

Choose a reason for hiding this comment

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

Hi Jon, could we also add a changelog entry?

@jsumiguin-causalens jsumiguin-causalens changed the title DO NOT MERGE YET - DOS-237 - Added props.onchange to the callback dependency Bugfix: DOS-237 - Added props.onchange to the callback dependency of numeric inputs and added onKeyDown property so we can bind to it Dec 19, 2023
@jsumiguin-causalens
Copy link
Contributor Author

@patricia-causalens, thanks, added now. Can you confirm if it's the correct format? Cheers. 🙏

@jsumiguin-causalens jsumiguin-causalens changed the title Bugfix: DOS-237 - Added props.onchange to the callback dependency of numeric inputs and added onKeyDown property so we can bind to it Fix: DOS-237 - Added props.onchange to the callback dependency of numeric inputs and added onKeyDown property so we can bind to it Dec 19, 2023
Copy link
Contributor

@patricia-causalens patricia-causalens left a comment

Choose a reason for hiding this comment

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

LGTM

packages/ui-components/src/numeric-input/numeric-input.tsx Outdated Show resolved Hide resolved
@patricia-causalens patricia-causalens merged commit c594ade into master Dec 21, 2023
2 checks passed
@patricia-causalens patricia-causalens deleted the DOS-437_numeric_input_deps_issue branch December 21, 2023 12:01
patricia-causalens pushed a commit that referenced this pull request Jan 5, 2024
…eric inputs and added onKeyDown property so we can bind to it (#58)

* Added props.onchange to the callback dependency as the onChange does not change in sync with the parent

* added tests and changelog

* fixed changelog

* removed if needed
patricia-causalens added a commit that referenced this pull request Jan 9, 2024
* merge node dragging bug

* merge soft edge fix

* Fix: DOS-237 - Added props.onchange to the callback dependency of numeric inputs and added onKeyDown property so we can bind to it (#58)

* Added props.onchange to the callback dependency as the onChange does not change in sync with the parent

* added tests and changelog

* fixed changelog

* removed if needed

* update version to 1.4.4

* updating changelogs

* support for tiers in spring layout

* support in marketing layout

* added missing changelog

* adjusting ordering force

* PR feedback

---------

Co-authored-by: Krzysztof Bielikowicz <[email protected]>
Co-authored-by: Jonathan Sumiguin <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants