[NumberField] Component and Hook #186

atomiks · 2024-03-15T12:04:44Z

I have followed (at least) the PR section of the contributing guide.

Closes #185
Closes #224

Preview: https://deploy-preview-186--base-ui.netlify.app/base-ui/react-number-field/

TODO:

Complete tests
Documentation

packages/mui-base/src/utils/useControlled.ts

packages/mui-base/src/utils/getElementDocument.ts

oliviertassinari · 2024-03-20T16:12:13Z

Rebased on HEAD. It seems to fix Circle CI, I don't know why.

michaldudak · 2024-03-22T11:13:52Z

It might be subjective, but the scrubber feels too sensitive. Changing value on every pixel of pointer movement may be too fast. Figma seems to update the value every two pixels (or perhaps they take pixel density into consideration?). We could make this configurable by adding the sensitivity prop to the ScrubArea (being the number of pixels the pointer should move before the value is updated)

atomiks · 2024-03-22T11:22:53Z

@michaldudak yeah you're right, previously it was 2px but I must have changed something that reverted it back to 1px. Will fix.

colmtuite

Tested on Chrome, Safari, Safari iOS, and Chrome iOS. Feedback/thoughts so far:

ScrubAreaCursor is missing from the docs anatomy section.
Seems like we don't have the teleport threshold thingy prop on ScrubAreaCursor yet? Are you planning to add that?
Should scrubbing start from where the pointer was when scrubbing began, or when the pointer direction changes during scrubbing? Idk how to explain wtf I'm on about here 🤣 It's noticeable when min and max are set. If you set max, then scrub way beyond the max value, you need to scrub all the way back down before the value decrements. Perhaps it could begin decrementing immediately when the pointer direction changes?
The largeStep logic seems odd to me. I expected it to just increment/decrement the value by whatever the largeStep value is?
smallStep doesn't seem to be working for me. Nothing happens when I hold cmd and then increment/decrement.

Still to test: formatting options.

mnajdova

Looks really good 👌

packages/mui-base/src/NumberField/NumberField.tsx

mnajdova · 2024-03-29T11:52:41Z

packages/mui-base/src/NumberField/NumberField.tsx

+    value,
+    onChange,
+    allowWheelScrub,
+    render: renderProp,


Suggested change

render: renderProp,

render = defaultRender,

I can't see renderProp being used anywhere else. I see this patterns used everywhere. If there isn't a valid reason I would propose going with this one.

We went with the ?? fallback because it avoids requiring a default when generating docs which is too difficult with the generic BaseUiCommonComponentProps due to the polymorphic components.

mnajdova · 2024-03-29T11:54:23Z

packages/mui-base/src/NumberField/NumberField.tsx

+  } = props;
+  const render = renderProp ?? defaultRender;
+
+  const numberField = useNumberField(props);


Suggested change

const numberField = useNumberField(props);

const numberField = useNumberField(props);

We usually invoke the hooks with the ownerState, this makes sure that the default values for the props are also included. It may not make a difference in this component, but if we change the default value of some prop, it may break unexpectedly.

In this case, ownerState is a partially derived version of useNumberField's return value, so it can't be called with it. The default values of the hook just match the component.

packages/mui-base/src/NumberField/NumberFieldGroup.tsx

mnajdova · 2024-03-29T12:05:10Z

packages/mui-base/src/utils/clamp.ts

@@ -0,0 +1 @@
+export { default as clamp } from '@mui/utils/clamp';


Can we do the utils changes in a separate PR? It will make the review easier.

I think at this stage, it's easier if the utils are kept in this PR. To keep it working before rebasing, I'd have to import directly from @mui/utils temporarily, right? I can remember to do utils separately for next time.

mnajdova · 2024-03-29T12:15:10Z

packages/mui-base/src/useNumberField/useNumberField.ts

+  // We need to hide the input value during SSR to avoid a potential mismatch between the server
+  // rendered value and the client rendered value when their locales differ.


Flash from server's formatting to the user's one. If the server matches the client, there's no flash. This indeed may be better.

👍

colmtuite · 2024-04-02T13:12:38Z

Decisions from a recent call:

Agreed to just change the scrub min/max thing. There was one other issue mentioned, but James couldn't remember what it was. So the plan is to change it, then test again.
Do some research to see if cmd/ctrl is the best key for smallStep. Perhaps alt/option is better?
Internationalisation: passing functions and stuff needs to be added.

atomiks · 2024-04-02T23:12:12Z

@colmtuite

Already changed
Given only Chakra seems to have this, I couldn't find any info about it. I don't think it matters really — I asked Claude and its first thought was Alt not Meta, if that has any bearing. But it said it doesn't really matter overall.

Internationalisation: passing functions and stuff needs to be added.

Can you elaborate more here? The format prop doesn't accept a function in that it only accepts Intl.NumberFormatOptions, but do you want to enable them to format the input however they like as well? (I believe Chakra does this.) I think this can be done if they specify value manually on NumberField.Input as it will override our internal prop, but this may cause issues (that I will need to investigate). If they do that as well, it will force them to make the component controlled since they'll need access to the number value.

We might be missing props similar to validate (accepts a function) and validationBehavior (string) from react-aria, however.

colmtuite · 2024-04-03T08:01:31Z

@atomiks

I asked Claude and its first thought was Alt

Ok yeah my first thought was Alt also. If that's the only info we have, and both point to Alt, I guess let's go with Alt.

do you want to enable them to format the input however they like as well

No this was just a note I took from our call, not sure what you meant when you were talking about formatting, internationalisation, and functions. I remember you saying some piece of work around internationalisation is still incomplete?

atomiks · 2024-04-03T08:12:01Z

@colmtuite

Ok yeah my first thought was Alt also. If that's the only info we have, and both point to Alt, I guess let's go with Alt.

Alright, will change 👍

I remember you saying some piece of work around internationalisation is still incomplete?

Yeah there were some bugs remaining with the parsing and validation functions, but I should have fixed most of them in the recent commits. However these are only internal and not part of any API.

michaldudak · 2024-04-09T11:13:21Z

docs/data/base/components/number-field/number-field.md

+  return (
+    <NumberField value={value} onChange={setValue}>
+      <NumberField.Group>
+        <NumberField.Decrement>&minus;</NumberField.Decrement>


I'm curious, why not -?

&minus is wider and looks more in proportion with + 😅

docs/data/base/components/number-field/number-field.md

michaldudak · 2024-04-09T11:28:53Z

packages/mui-base/src/useNumberField/useScrub.ts

+import { mergeReactProps } from '../utils/mergeReactProps';
+
+/**
+ * @ignore - internal hook.


Don't we want to expose it like the other hooks?

No as it's just an internal hook to place some of the functionality in its own hook

packages/mui-base/src/NumberField/utils/validate.test.ts

packages/mui-base/src/useNumberField/useNumberField.ts

Bifurcate tests Docs stylelint Attempt test fix Ignore perf test in JSDOM Use skip Docs Format Update docs Relocate ScrubArea logic Memo return value Unit tests for parsing and formatting Skip test in browser Tests Relocate pointerDownEvent Remove floating point errors Docs Update allowed chars Bug fixes and feedback Remove autoFocus destructure Remove clamp requirement logic Allow currency symbols Move context menu handler Avoid calling handlers while disabled Unsubscribe from visualViewport Add description to useNumberField Docs Use mergeReactProps Use altKey for smallStep Fix cursor rendering

atomiks marked this pull request as draft March 15, 2024 12:04

atomiks added the component: number field This is the name of the generic UI component, not the React module! label Mar 15, 2024

colmtuite requested a review from a team March 15, 2024 13:22

flaviendelangle reviewed Mar 15, 2024

View reviewed changes

packages/mui-base/src/utils/useControlled.ts Outdated Show resolved Hide resolved

packages/mui-base/src/utils/getElementDocument.ts Outdated Show resolved Hide resolved

atomiks force-pushed the feat/NumberField branch 4 times, most recently from 6992248 to 023fe9d Compare March 18, 2024 06:10

oliviertassinari force-pushed the feat/NumberField branch from 585d357 to c0f561a Compare March 20, 2024 16:07

atomiks closed this Mar 20, 2024

atomiks deleted the feat/NumberField branch March 20, 2024 20:25

atomiks restored the feat/NumberField branch March 20, 2024 20:26

atomiks reopened this Mar 20, 2024

atomiks force-pushed the feat/NumberField branch 10 times, most recently from 71b21fe to 4aa63f6 Compare March 22, 2024 10:37

oliviertassinari added breaking change and removed breaking change labels Mar 22, 2024

oliviertassinari mentioned this pull request Mar 22, 2024

[number field] Disable focus on mobile #224

Closed

colmtuite requested changes Mar 22, 2024

View reviewed changes

atomiks marked this pull request as ready for review March 29, 2024 03:59

mnajdova reviewed Mar 29, 2024

View reviewed changes

atomiks force-pushed the feat/NumberField branch 2 times, most recently from 99d4bb0 to 7866e25 Compare April 2, 2024 08:25

atomiks mentioned this pull request Apr 4, 2024

[core] Adjust conformance tests to support portals #266

Merged

1 task

colmtuite self-requested a review April 8, 2024 08:08

colmtuite approved these changes Apr 8, 2024

View reviewed changes

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 9, 2024

michaldudak reviewed Apr 9, 2024

View reviewed changes

atomiks force-pushed the feat/NumberField branch from 75cf7b3 to c8cc076 Compare April 9, 2024 11:53

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 9, 2024

atomiks added 5 commits April 9, 2024 21:56

Feedback

8c89dbe

Fix import names

8c4cf84

Remove NumberInput

6fe4968

Fix imports

2bd42ba

Sync docs

4e66442

michaldudak approved these changes Apr 10, 2024

View reviewed changes

Fix initial focus selection

1c231b4

atomiks force-pushed the feat/NumberField branch from c618708 to 1c231b4 Compare April 10, 2024 13:44

Parsing adjustments

4e180da

michaldudak mentioned this pull request Apr 10, 2024

[release] 1.0.0-alpha.0 #291

Merged

Minor refactors

091837a

atomiks merged commit f68970d into mui:master Apr 11, 2024
16 checks passed

atomiks deleted the feat/NumberField branch April 11, 2024 02:21

michaldudak pushed a commit to michaldudak/base-ui that referenced this pull request Apr 11, 2024

[NumberField] Component and Hook (mui#186)

b67e7bd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NumberField] Component and Hook #186

[NumberField] Component and Hook #186

atomiks commented Mar 15, 2024 •

edited

oliviertassinari commented Mar 20, 2024

michaldudak commented Mar 22, 2024

atomiks commented Mar 22, 2024

colmtuite left a comment

mnajdova left a comment

mnajdova Mar 29, 2024

atomiks Mar 29, 2024

mnajdova Mar 29, 2024

atomiks Mar 31, 2024 •

edited

mnajdova Mar 29, 2024

atomiks Mar 31, 2024

mnajdova Mar 29, 2024

colmtuite commented Apr 2, 2024

atomiks commented Apr 2, 2024 •

edited

colmtuite commented Apr 3, 2024

atomiks commented Apr 3, 2024

michaldudak Apr 9, 2024

atomiks Apr 9, 2024

michaldudak Apr 9, 2024

atomiks Apr 9, 2024

	const numberField = useNumberField(props);
	const numberField = useNumberField(props);

		@@ -0,0 +1 @@
		export { default as clamp } from '@mui/utils/clamp';

		// We need to hide the input value during SSR to avoid a potential mismatch between the server
		// rendered value and the client rendered value when their locales differ.

[NumberField] Component and Hook #186

[NumberField] Component and Hook #186

Conversation

atomiks commented Mar 15, 2024 • edited

oliviertassinari commented Mar 20, 2024

michaldudak commented Mar 22, 2024

atomiks commented Mar 22, 2024

colmtuite left a comment

Choose a reason for hiding this comment

mnajdova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atomiks Mar 31, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colmtuite commented Apr 2, 2024

atomiks commented Apr 2, 2024 • edited

colmtuite commented Apr 3, 2024

atomiks commented Apr 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atomiks commented Mar 15, 2024 •

edited

atomiks Mar 31, 2024 •

edited

atomiks commented Apr 2, 2024 •

edited