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

refactor(number-input): Separation of NumberInput and Input #200

Merged
merged 9 commits into from
Oct 13, 2022

Conversation

yasincaliskan
Copy link
Contributor

@yasincaliskan yasincaliskan commented Jul 8, 2022

Description

  • Created NumberInput component and moved the related props, utilities, tests from Input to NumberInput.
  • Localization calculations are separated into 2 main utilities as localizeNumberInputValue and delocalizeNumberInputValue in order to clean up the component body and change handler.

@yasincaliskan yasincaliskan added the W.I.P Work in progress label Jul 8, 2022
@yasincaliskan yasincaliskan self-assigned this Jul 8, 2022
Copy link
Collaborator

@jamcry jamcry left a comment

Choose a reason for hiding this comment

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

Thanks Yasin 👍

@yasincaliskan yasincaliskan requested a review from jamcry July 18, 2022 09:56
@yasincaliskan yasincaliskan requested a review from jamcry July 22, 2022 11:31
Copy link
Collaborator

@jamcry jamcry left a comment

Choose a reason for hiding this comment

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

Thanks Yasin, I think this is good to go 🚀
Though, I believe it'd be nice to get this tested on mobile and old unsupported browsers before including it in the next release. (To make sure our checks are working and nothing is broken on those browsers)
Maybe we can push this branch to somewhere (netlify/github pages) and request QA to somehow test this, or we can test it ourselves using our devices and CrossBrowserTesting app

@github-actions
Copy link

github-actions bot commented Sep 2, 2022

PR Preview Action v1.1.1
Preview removed because the pull request was closed.
2022-10-13 13:02 UTC

Copy link
Collaborator

@jamcry jamcry left a comment

Choose a reason for hiding this comment

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

Good news: now we have PR preview links
Bad news: Unsupported browsers still cause problems 😅

I briefly tried to debug this. Although we don't directly use getNumberSeparators function, some other utils use it internally. (For example parseNumber). And we use it inside some number input utils, which are called for all type of devices. I think that's the problem.

Screenshot 2022-09-02 at 13 51 51

@yasincaliskan
Copy link
Contributor Author

I love the PR review! Thanks Cem 🎉
I improved the Intl API checks, and it is not broken now on unsupported browsers.

@yasincaliskan yasincaliskan merged commit 10cca82 into next-release Oct 13, 2022
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.

4 participants