Skip to content

fix(Input): Does not update value as expected #1782

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

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

olehnoskov-cengage
Copy link
Collaborator

@olehnoskov-cengage olehnoskov-cengage commented May 14, 2025

Closes: #1721

What I did

  • Provided a fix for this issue on the customer side.
  • Added example in Storybook.
  • Added additional tests of using Input on the customer side to track break changes.

Screenshots

Screencast.from.11.04.25.10.50.08.webm

Storybook:
image
Screenshot from 2025-05-20 11-03-25

Checklist

  • changeset has been added
  • Pull request is assigned, labels have been added and ticket is linked
  • Pull request description is descriptive and testing steps are listed
  • Corresponding changes to the documentation have been made
  • New and existing unit tests pass locally with the proposed changes
  • Tests that prove the fix is effective or that the feature works have been added

How to test

  • Go to Storybook
  • Input -> Time Input
  • Type 133, 66 or 333 into the Minutes field (RM input)
  • In case of 66, in Hours field should be 1 hour and in Minutes should be 6
  • The values of minutes and hours have to be the same for the native input component.

On the customer side need add this line to fix this issue:

isHours ? setHours(currentUnitValue) : setMinutes(currentUnitValue);

You can take a look at the implementation onChangeTimedDuration function

Why should the customer update the current Implementation?

The main issue lies in this line: const currentUnitValue = numericValue && numericValue >= 0 ? numericValue : 0;

Let’s say numericValue === 66. Based on totalDurationMilliseconds, the parsed minutes become 6, so we call setMinutes(6). However, if the current state minutes is already 6, React sees no change and skips the re-render.

As a result:

  1. Even though the user typed 66, the internal minutes state is 6
  2. React skips the update as a no-op (minutes didn’t change)
  3. The input still shows 66, but logically the time is 1h 6m, causing a mismatch.

Why This Works Differently in Native Inputs vs. React Magma Input

Native inputs update their displayed value immediately, independent of React state — they're "uncontrolled" in that sense. In contrast, InputBase and Input components in React Magma are state-driven and semi-controlled. Their displayed value is tied to React state and props.

If the parent component fails to re-set the value explicitly, or if React optimizes away a no-op update, the visual value may fall out of sync with the actual state — especially when parsing alters the value (like converting 66 to 6).

Copy link

changeset-bot bot commented May 14, 2025

🦋 Changeset detected

Latest commit: c54923c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
react-magma-dom Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@silvalaura
Copy link
Collaborator

What changes were made after reverting it? Do we know the root cause of the previous issue?

@olehnoskov-cengage
Copy link
Collaborator Author

What changes were made after reverting it? Do we know the root cause of the previous issue?

As @moathabuhamad-cengage explained to me, maybe these changes added some breaking change in other Cengage repos that's way it was reverted, I am right, Moa?

Copy link
Contributor

Copy link
Contributor

@silvalaura
Copy link
Collaborator

What changes were made after reverting it? Do we know the root cause of the previous issue?

As @moathabuhamad-cengage explained to me, maybe these changes added some breaking change in other Cengage repos that's way it was reverted, I am right, Moa?

Right. We don't want to have breaking changes, so we need to investigate what about this pull request is causing our adopters to break before we merge it back.

@olehnoskov-cengage olehnoskov-cengage added the draft Pull requests that are drafts and not ready for review label May 14, 2025
@moathabuhamad-cengage moathabuhamad-cengage force-pushed the bugfix/InputDoesNotUpdateValue branch from 4f75040 to c5eb083 Compare May 16, 2025 12:26
@moathabuhamad-cengage
Copy link
Collaborator

@olehnoskov-cengage I added a storybook example and a few tests to match the errors we saw in the adopters repo, now you can fix the ticket with these included. those tests pass on current dev but fail on this branch

image

Copy link
Contributor

Copy link
Contributor

@olehnoskov-cengage olehnoskov-cengage added ready for review Pull requests that are ready for developer review and removed draft Pull requests that are drafts and not ready for review labels May 19, 2025
Copy link
Contributor

Copy link
Contributor

Copy link
Collaborator

@silvalaura silvalaura left a comment

Choose a reason for hiding this comment

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

Just curious-- I am seeing new tests, but no changes to the input component. Do we know the root cause of what caused our adopter's tests to fail with the code changes we introduced?

@olehnoskov-cengage
Copy link
Collaborator Author

olehnoskov-cengage commented May 26, 2025

Just curious-- I am seeing new tests, but no changes to the input component. Do we know the root cause of what caused our adopter's tests to fail with the code changes we introduced?

Yes, @moathabuhamad-cengage suggested adding new tests to monitor breaking changes on the adopter's side.
The main issue of failing tests was that in tests we were waiting for text like text, but in handleChange during every keystroke we called this function and in result we had a result like: t, e, x, t because we passed in props new value every keystroke. That's why it worked well visually, but tests failed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review Pull requests that are ready for developer review
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants