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 field-editor [BAU-722] #1203

Merged
merged 8 commits into from
Aug 22, 2022
Merged

Conversation

bgutsol
Copy link
Contributor

@bgutsol bgutsol commented Jul 28, 2022

  • Use type="text" instead of type="number", see why

  • Number field editor does not allow entering nun-numeric input, like a native input type="number"

  • Number field editor has step buttons, like a native input type="number"

  • Arrow keys can be used to step as well, like a native input type="number"

  • Don't use inputmode=decimal/numeric because some keyboards on mobile devices don't allow typing minus - or dot .. For example, Safari on IOS.

Related PRs:

@netlify
Copy link

netlify bot commented Jul 28, 2022

Deploy Preview for contentful-field-editors ready!

Name Link
🔨 Latest commit b849ea9
🔍 Latest deploy log https://app.netlify.com/sites/contentful-field-editors/deploys/63037f2252b82a0009ab2021
😎 Deploy Preview https://deploy-preview-1203--contentful-field-editors.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@bgutsol bgutsol marked this pull request as ready for review August 1, 2022 07:53
@bgutsol bgutsol requested a review from a team as a code owner August 1, 2022 07:53
@bgutsol bgutsol requested review from denkristoffer, burakukula, mshaaban0, massao and a team and removed request for a team August 1, 2022 07:53
Copy link
Contributor

@massao massao left a comment

Choose a reason for hiding this comment

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

LGTM, should we consider adding this to forma??

@@ -24,9 +24,7 @@
"@contentful/f36-components": "^4.0.27",
"@contentful/f36-tokens": "^4.0.0",
"@contentful/field-editor-shared": "^1.1.3",
"emotion": "^10.0.17",
"lodash": "^4.17.15",
"lodash-es": "^4.17.15"
Copy link
Contributor

Choose a reason for hiding this comment

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

🥇

@bgutsol
Copy link
Contributor Author

bgutsol commented Aug 1, 2022

LGTM, should we consider adding this to forma??

Yes 💯

@bgutsol bgutsol requested a review from MayaGillilan August 1, 2022 12:34
Copy link
Contributor

@mshaaban0 mshaaban0 left a comment

Choose a reason for hiding this comment

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

LGTM. Not sure about the version release, is this a breaking change?

Copy link
Contributor

@giotiskl giotiskl 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 overall, my main suggestion would be to rely on Number.isNaN instead of isNaN, explanation in the comments.

packages/number/src/NumberEditor.tsx Outdated Show resolved Hide resolved
packages/number/src/NumberEditor.tsx Outdated Show resolved Hide resolved
packages/number/src/NumberEditor.tsx Outdated Show resolved Hide resolved
packages/number/src/NumberEditor.tsx Outdated Show resolved Hide resolved
packages/number/src/parseNumber.ts Outdated Show resolved Hide resolved
@bgutsol
Copy link
Contributor Author

bgutsol commented Aug 5, 2022

Looks good overall, my main suggestion would be to rely on Number.isNaN instead of isNaN, explanation in the comments.

@giotiskl thanks for reviewing! I've addressed your comments, please take a look

@bgutsol bgutsol changed the title refactor: Number field-editor [BAU-722] [DON'T MERGE] refactor: Number field-editor [BAU-722] Aug 5, 2022
@bgutsol bgutsol changed the title [DON'T MERGE] refactor: Number field-editor [BAU-722] refactor: Number field-editor [DON'T MERGE] [BAU-722] Aug 5, 2022
@bgutsol bgutsol changed the title refactor: Number field-editor [DON'T MERGE] [BAU-722] refactor: Number field-editor ❗️DON'T MERGE❗️ [BAU-722] Aug 5, 2022
@bgutsol bgutsol force-pushed the refactor/number-editor branch from e221441 to 51ad4af Compare August 5, 2022 12:20
Copy link
Contributor

@denkristoffer denkristoffer left a comment

Choose a reason for hiding this comment

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

What does it look like?

packages/number/src/NumberEditor.tsx Outdated Show resolved Hide resolved
@bgutsol
Copy link
Contributor Author

bgutsol commented Aug 17, 2022

What does it look like?

@denkristoffer you can take a look on preview https://deploy-preview-1203--contentful-field-editors.netlify.app/number

@denkristoffer
Copy link
Contributor

Looks good, how about some hover and active styling on the buttons though?

@bgutsol
Copy link
Contributor Author

bgutsol commented Aug 17, 2022

Looks good, how about some hover and active styling on the buttons though?

Active styling is in place. With hover I agree, we can add some

@denkristoffer
Copy link
Contributor

Active styling is in place. With hover I agree, we can add some

I mean the clicked state, I'm not seeing it:

Screen.Recording.2022-08-18.at.14.04.46.mov

@bgutsol
Copy link
Contributor Author

bgutsol commented Aug 18, 2022

Active styling is in place. With hover I agree, we can add some

I mean the clicked state, I'm not seeing it:

Screen.Recording.2022-08-18.at.14.04.46.mov

The current active style doesn't work in firefox for some reason 🤔

@bgutsol
Copy link
Contributor Author

bgutsol commented Aug 22, 2022

Active styling is in place. With hover I agree, we can add some

I mean the clicked state, I'm not seeing it:
Screen.Recording.2022-08-18.at.14.04.46.mov

The current active style doesn't work in firefox for some reason 🤔

Unfortunately, there is no easy fix for it, it's just the way Firefox implemented its core logic, and it's different from the other browsers.
So basically event.preventDefault();, that I call within onPointerDown callback, cancels the active state for our buttons (only in Firefox). But I can't remove the event.preventDefault(); because I need it to switch the focus from the buttons back to the input for a11y purposes. So the only way to fix it is by setting our custom .active class. But it doesn't worth the effort, in my opinion. What are your thoughts @denkristoffer?

@bgutsol bgutsol requested a review from denkristoffer August 22, 2022 12:34
@bgutsol bgutsol changed the title refactor: Number field-editor ❗️DON'T MERGE❗️ [BAU-722] refactor: Number field-editor [BAU-722] Aug 22, 2022
@bgutsol bgutsol merged commit d93698e into master Aug 22, 2022
@bgutsol bgutsol deleted the refactor/number-editor branch August 22, 2022 13:51
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.

5 participants