Number field revamp #2566

finnar-bin · 2024-02-22T08:17:51Z

number-field.webm

…ent/number-field-revamp

package.json

agalin920 · 2024-02-27T00:12:22Z

src/shell/components/FieldTypeNumber.tsx

+  const modifyNumberValue = (action: "increment" | "decrement") => {
+    switch (action) {
+      case "increment":
+        onChange(+(+value + 1).toFixed(10), name);


we always storing 10 decimal places?

I've updated how the increment/decrement works to make sure decimals are saved properly without making use of toFixed

agalin920 · 2024-02-27T07:28:13Z

@zcolah I have concerns with the auto comma functionality. Not only is it requiring a technical overhead with a dependency needed to be added but it brings up several questions especially in the context of a CMS.

Primarily how does a user know whether the number field value is being saved with or without the commas?

Also accessibility concerns:

Not all regions make use of comma separators in the same way. (e.g periods are used to separate thousands and commas for decimals)
Screen reader cannot read this number properly

zcolah · 2024-02-27T12:00:55Z

@agalin920

> Primarily how does a user know whether the number is being saved with or without the commas?
Good question. We will add an information note to the number field docs to mention that they do not get saved with commas. We have multiple fields where what the marketer sees is not what the developer sees and this will be similar to that. e.g. The WYSIWYG field, the Boolean field, etc.

> 1. Not all regions make use of comma separators in the same way. (e.g periods are used to separate thousands and commas for decimals)
This is a fair point and was considered. Since we are a U.S. based company with a majority of our clients also U.S. based, we decided to follow the US Number format. That being said in the future, if the need arises we will add the functionality for a user to define their number format. We have decided that this is out of scope for this project.

> 2. Screen reader cannot read this number properly
Could you provide me with more context as to why you believe it cannot read the number properly? What is preventing it from happening? Any Loom video or supporting evidence for it would be appreciated.

Also could you recommend a solution towards how we can make a screen reader read the number properly?

zcolah · 2024-02-27T12:06:49Z

@agalin920 I also wanted to give some key context on why the number separators (commas) are being introduced.

Currently when a user inputs a larger number (e.g. 1000000000) they tend to have difficulty reading and understanding what the value is. This leads to a user potentially misreading the number or adding an extra zero or a lesser zero by mistake.
This issue can be very concerning for an insurance company or any large enterprise company where data integrity is very critical.

See issue: #2456

zcolah · 2024-02-27T12:17:54Z

@agalin920 @theofficialnar I also do not have full context on why the entire solution being used to achieve the commas is bad but here are some discussions I have found online that discuss how others have achieved this. Maybe they could be helpful. Let me know what I can do to unblock this issue moving forward.

agalin920 · 2024-02-27T18:41:10Z

@agalin920 I also wanted to give some key context on why the number separators (commas) are being introduced.

Currently when a user inputs a larger number (e.g. 1000000000) they tend to have difficulty reading and understanding what the value is. This leads to a user potentially misreading the number or adding an extra zero or a lesser zero by mistake.

This issue can be very concerning for an insurance company or any large enterprise company where data integrity is very critical.

See issue: #2456

@zcolah while I understand the pain points you present I fundamentally disagree with the solution and believe the trade offs of ambiguous comma separation is worse. I agree with the html numeric input standard to not comma separate and even data entry software like sheets or excel also does not by default auto comma numbers. It makes data entry standard and in my opinion comma separation should be used for display purposes only and not data entry.

Ultimately It also doesn’t follow the web standard

zcolah · 2024-02-27T18:56:20Z

@agalin920

data entry software like sheets or excel also does not by default auto comma numbers

For all cells that are number types, Excel adds commas and decimal places actually by default. It does not add commas for plain text (string type) cells. In fact in most calculator interfaces as well commas are added by default in the number input.

should be used for display purposes only

The Content App, displays to a marketer, critical number values in their content items during content creation and content review/editing.

During content creation, it is important for us to display the number they are inputting in the most readable easy to scan format so as to ensure they do not make a mistake. For example it can be a question of them inputting 100,000 vs 1,000,000 for a critical page.

During content review and editing, we need to make it as easy as possible for them to read a number in a number input.
Compare reading and scanning 1,000,000,000 vs 1000000000. A user can be more likely to make a mistake reading the number without commas.

zcolah · 2024-02-27T19:14:38Z

@agalin920 @theofficialnar
I do want to help. I found these two Codepen examples. Potentially we could see how they did it and maybe then not use an external package/dependency?

https://codepen.io/vladracoare/pen/VwLRqRy
https://codepen.io/tsunet111/pen/GbpwZa

finnar-bin · 2024-02-27T22:26:37Z

@zcolah How about having some kind of preview of the number they have typed with comma separation while maintaining how the actual input already is?

agalin920 · 2024-02-28T00:11:32Z

@agalin920 @theofficialnar I do want to help. I found these two Codepen examples. Potentially we could see how they did it and maybe then not use an external package/dependency?

https://codepen.io/vladracoare/pen/VwLRqRy https://codepen.io/tsunet111/pen/GbpwZa

Not really looking for solution examples, even without a direct dependency they all make use of JS to manipulate the input. The main flag im raising is html number input standard deviation.

@zcolah How about having some kind of preview of the number they have typed with comma separation while maintaining how the actual input already is?

Yea suggested this as well, to maintain numeric input standard.

zcolah

@theofficialnar caught a new bug related to decimal places in VQA which did not exist before: https://www.loom.com/share/b14736392b2940ec83fc86c158895510

Sorry for the delay on the VQA for this as I was in the hospital

zcolah · 2024-03-12T08:50:54Z

@theofficialnar the issue does not seem to be resolved with the fix that is pushed to stage
https://www.loom.com/share/2d74c9c2fa32413f8af785a09b634519?from_recorder=1&focus_title=1

zcolah

Hi @theofficialnar

A new bug has crept in since I last checked the number field. This time I cannot remove a decimal place if there is only a 0 right after it. Please see Loom for more context:

https://www.loom.com/share/b1d1d0f45bce45c3bf53757a69b01b49

zcolah · 2024-03-13T22:02:54Z

@theofficialnar caught a new bug related to decimal places in VQA which did not exist before: https://www.loom.com/share/b14736392b2940ec83fc86c158895510

did you see this btw @theofficialnar

finnar-bin · 2024-03-13T22:50:39Z

@theofficialnar caught a new bug related to decimal places in VQA which did not exist before: https://www.loom.com/share/b14736392b2940ec83fc86c158895510

did you see this btw @theofficialnar

Yeah, this should already be fixed with the changes yesterday

zcolah

@theofficialnar

I cannot remove 0 if it is the last digit in the decimal places: https://www.loom.com/share/53e547912e604b93a2a12123531101e2
I cannot remove 0 if it is the last digit in the whole number places: https://www.loom.com/share/87cb53a9df684d37ad8533cf7ac02f5f

zcolah

Wrong comment. Ignore.

agalin920 · 2024-03-14T15:18:05Z

src/shell/components/FieldTypeNumber.tsx

+    }
+  };
+
+  const adjustCursorPosition = (numStr: string) => {


The complexity of this is getting very large. Is there no alternative?

Its also hard to now see the benefit of the library since its not solving much complexity

I’m actually no longer using the react-number format library here hence all the complexity and bugs since I have to handle all the cursor position etc manually. I’m actually considering using the library again since it takes care of all the formatting and resolves all of the bugs @zcolah has been raising for the past couple of days. Only downside to that library being that it automatically converts the exponents to numbers with leading 0s instead of showing the actual exponent.

giseleblair · 2024-03-14T17:00:25Z

@giseleblair where are you seeing it being saved as a string? I checked just now and it's always saving it as a number.

I am seeing that the number is being returned as an integer in the API. Looks like webengine is converting these to string in our read-only APIs. This is something we need to confirm with @jjaguilar08

finnar-bin · 2024-03-14T21:58:03Z

@zcolah @agalin920 what are your thoughts on bringing back in the library which handles the number formatting etc? That library resolves all the issues that are being brought up and the complexity of this component, only issue is that it automatically converts large exponent numbers to numbers with trailing 0s instead of showing the actual exponent.

agalin920 · 2024-03-14T23:15:11Z

@theofficialnar thats fine by me

zcolah · 2024-03-15T08:25:46Z

@theofficialnar fine by me as well

zcolah · 2024-03-15T08:29:54Z

@theofficialnar the Decimal places get rounded when increasing the number with the plus button. Is it possible to ensure that does not happen?

https://www.loom.com/share/0f4218027df54907a55d53e73ed0ddcb?from_recorder=1&focus_title=1

finnar-bin · 2024-03-18T06:08:02Z

@zcolah turns out it's a limitation with Javascript's floating-point precision hence it's getting rounded off, javascript numbers can hold 15 digits accurately

zcolah · 2024-03-18T12:06:19Z

@theofficialnar Thank you for that insight. It is fair to not resolve it but have a few follow up questions regarding the scope of this javascript constraint.

Does this impact only a situation where a user tries to increase the value with the plus and minus buttons?
Does this mean any value that has more than 15 digits will get rounded post that and saved like that in the database?
What happens in a situation where the database returns a value with more than 15 digits? Does the input round it.

finnar-bin · 2024-03-18T23:28:03Z

@zcolah

Does this impact only a situation where a user tries to increase the value with the plus and minus buttons?

As of the moment, user input gets retained but the value saved on the database is rounded up.

Does this mean any value that has more than 15 digits will get rounded post that and saved like that in the database?

Yes, unless they start exceeding the MAX_SAFE_INTEGER then they'll be converted to exponents

What happens in a situation where the database returns a value with more than 15 digits? Does the input round it.

Yes

zcolah · 2024-03-19T18:51:31Z

@zcolah

Does this impact only a situation where a user tries to increase the value with the plus and minus buttons?

As of the moment, user input gets retained but the value saved on the database is rounded up.

Does this mean any value that has more than 15 digits will get rounded post that and saved like that in the database?

Yes, unless they start exceeding the MAX_SAFE_INTEGER then they'll be converted to exponents

What happens in a situation where the database returns a value with more than 15 digits? Does the input round it.

Yes

Thank you for these insights. I am going to give it a VQA label now.

finnar-bin added 9 commits February 13, 2024 13:47

task: use react-number-format

cda7c62

Merge branch 'master' of github.com:zesty-io/manager-ui into enhancem…

af4ce52

…ent/number-field-revamp

Merge branch 'master' into enhancement/number-field-revamp

827c203

task: add decrement and increment buttons

45cf9d4

task: simplify increment/decrement functions

da20b56

task: default to 0

428790f

task: preserve cursor location when value defaults to 0

5038ee1

task: created separate FieldTypeNumber component

4776588

chore: cleanup

bd7cf88

finnar-bin linked an issue Feb 22, 2024 that may be closed by this pull request

Content: Number Field Revamp #2456

Closed

finnar-bin self-assigned this Feb 25, 2024

finnar-bin marked this pull request as ready for review February 25, 2024 23:08

finnar-bin requested a review from agalin920 February 25, 2024 23:09

task: update test

bb797ce

finnar-bin added enhancement Improvement to an existing feature ready PR is complete and ready for deployment labels Feb 26, 2024

agalin920 requested changes Feb 27, 2024

View reviewed changes

task: change increment/decrement logic

54a6393

finnar-bin requested a review from agalin920 February 27, 2024 06:23

finnar-bin changed the title ~~Enhancement/number field revamp~~ Number field revamp Feb 27, 2024

agalin920 approved these changes Feb 27, 2024

View reviewed changes

zcolah suggested changes Mar 11, 2024

View reviewed changes

agalin920 approved these changes Mar 11, 2024

View reviewed changes

zcolah reviewed Mar 13, 2024

View reviewed changes

zcolah suggested changes Mar 14, 2024

View reviewed changes

agalin920 reviewed Mar 14, 2024

View reviewed changes

Merge branch 'master' into enhancement/number-field-revamp

682afbe

finnar-bin force-pushed the enhancement/number-field-revamp branch from fab3d66 to 978835e Compare March 15, 2024 02:31

task: treat number values as numbers instead of strings

5045420

finnar-bin requested a review from agalin920 March 15, 2024 02:59

agalin920 approved these changes Mar 15, 2024

View reviewed changes

zcolah added the vqa VQA is complete and approved label Mar 19, 2024

finnar-bin added 2 commits March 20, 2024 10:05

Merge branch 'master' into enhancement/number-field-revamp

19de103

Merge branch 'master' into enhancement/number-field-revamp

60ed58b

shrunyan changed the base branch from master to dev March 25, 2024 18:48

Merge branch 'dev' into enhancement/number-field-revamp

501a39d

shrunyan merged commit 9ad14c2 into dev Mar 27, 2024
1 check failed

shrunyan deleted the enhancement/number-field-revamp branch March 27, 2024 02:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Number field revamp #2566

Number field revamp #2566

finnar-bin commented Feb 22, 2024 •

edited

Loading

agalin920 Feb 27, 2024

finnar-bin Feb 27, 2024

agalin920 commented Feb 27, 2024 •

edited

Loading

zcolah commented Feb 27, 2024 •

edited

Loading

zcolah commented Feb 27, 2024 •

edited

Loading

zcolah commented Feb 27, 2024

agalin920 commented Feb 27, 2024 •

edited

Loading

zcolah commented Feb 27, 2024 •

edited

Loading

zcolah commented Feb 27, 2024

finnar-bin commented Feb 27, 2024 •

edited

Loading

agalin920 commented Feb 28, 2024 •

edited

Loading

zcolah left a comment •

edited by finnar-bin

Loading

zcolah commented Mar 12, 2024 •

edited by finnar-bin

Loading

zcolah left a comment •

edited by finnar-bin

Loading

zcolah commented Mar 13, 2024 •

edited by finnar-bin

Loading

finnar-bin commented Mar 13, 2024 •

edited

Loading

zcolah left a comment •

edited by finnar-bin

Loading

zcolah left a comment •

edited

Loading

agalin920 Mar 14, 2024 •

edited

Loading

finnar-bin Mar 14, 2024

giseleblair commented Mar 14, 2024

finnar-bin commented Mar 14, 2024

agalin920 commented Mar 14, 2024

zcolah commented Mar 15, 2024

zcolah commented Mar 15, 2024 •

edited

Loading

finnar-bin commented Mar 18, 2024

zcolah commented Mar 18, 2024

finnar-bin commented Mar 18, 2024 •

edited

Loading

zcolah commented Mar 19, 2024

Number field revamp #2566

Number field revamp #2566

Conversation

finnar-bin commented Feb 22, 2024 • edited Loading

agalin920 Feb 27, 2024

Choose a reason for hiding this comment

finnar-bin Feb 27, 2024

Choose a reason for hiding this comment

agalin920 commented Feb 27, 2024 • edited Loading

zcolah commented Feb 27, 2024 • edited Loading

zcolah commented Feb 27, 2024 • edited Loading

zcolah commented Feb 27, 2024

agalin920 commented Feb 27, 2024 • edited Loading

zcolah commented Feb 27, 2024 • edited Loading

zcolah commented Feb 27, 2024

finnar-bin commented Feb 27, 2024 • edited Loading

agalin920 commented Feb 28, 2024 • edited Loading

zcolah left a comment • edited by finnar-bin Loading

Choose a reason for hiding this comment

zcolah commented Mar 12, 2024 • edited by finnar-bin Loading

zcolah left a comment • edited by finnar-bin Loading

Choose a reason for hiding this comment

zcolah commented Mar 13, 2024 • edited by finnar-bin Loading

finnar-bin commented Mar 13, 2024 • edited Loading

zcolah left a comment • edited by finnar-bin Loading

Choose a reason for hiding this comment

zcolah left a comment • edited Loading

Choose a reason for hiding this comment

agalin920 Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

finnar-bin Mar 14, 2024

Choose a reason for hiding this comment

giseleblair commented Mar 14, 2024

finnar-bin commented Mar 14, 2024

agalin920 commented Mar 14, 2024

zcolah commented Mar 15, 2024

zcolah commented Mar 15, 2024 • edited Loading

finnar-bin commented Mar 18, 2024

zcolah commented Mar 18, 2024

finnar-bin commented Mar 18, 2024 • edited Loading

zcolah commented Mar 19, 2024

finnar-bin commented Feb 22, 2024 •

edited

Loading

agalin920 commented Feb 27, 2024 •

edited

Loading

zcolah commented Feb 27, 2024 •

edited

Loading

zcolah commented Feb 27, 2024 •

edited

Loading

agalin920 commented Feb 27, 2024 •

edited

Loading

zcolah commented Feb 27, 2024 •

edited

Loading

finnar-bin commented Feb 27, 2024 •

edited

Loading

agalin920 commented Feb 28, 2024 •

edited

Loading

zcolah left a comment •

edited by finnar-bin

Loading

zcolah commented Mar 12, 2024 •

edited by finnar-bin

Loading

zcolah left a comment •

edited by finnar-bin

Loading

zcolah commented Mar 13, 2024 •

edited by finnar-bin

Loading

finnar-bin commented Mar 13, 2024 •

edited

Loading

zcolah left a comment •

edited by finnar-bin

Loading

zcolah left a comment •

edited

Loading

agalin920 Mar 14, 2024 •

edited

Loading

zcolah commented Mar 15, 2024 •

edited

Loading

finnar-bin commented Mar 18, 2024 •

edited

Loading