Feat: improve formatBytes function #6427

Isaac-alencar · 2022-10-14T22:58:48Z

What does this PR do?

This PR improve formatBytes function by making it more dynamic. Based on OS user is on we change the conversion factor
from 1000 to 1024.

FYI:
Windows assumes that there are 1024 Bytes in a Kilobyte unit, and 1024 Kilobytes in a Megabyte unit etc.
while Ubuntu and Mac assumes, a 1000 bytes constitute a Kilobyte (KB) unit, 1000 Kilobytes for a Megabyte (MB) and so on.

That is, same file on ubuntu and mac are larger than windows

For example:

213066 ÷ 1024 = 208072 bites
213066 ÷ 1000 = 213066 bites

So, if you try to attach same file in different OS you're gonna get an error

Where should the reviewer start?

src/js/components/FileInput/FileInput.js

What testing has been done on this PR?

How should this be manually tested?

Do Jest tests follow these best practices?

screen is used for querying.
The correct query is used. (Refer to this list of queries)
userEvent is used in place of fireEvent.
asFragment() is used for snapshot testing.

Any background context you want to provide?

What are the relevant issues?

closes #5807

Screenshots (if appropriate)

Do the grommet docs need to be updated?

Should this PR be mentioned in the release notes?

Is this change backwards compatible or is it a breaking change?

jcfilben

I think we need to use something besides userAgentData to get the current operating system

src/js/components/FileInput/FileInput.js

Isaac-alencar · 2022-10-19T14:38:17Z

@jcfilben thanks for reviewing this, I'll work to fix it

Isaac-alencar · 2022-10-19T15:10:53Z

@jcfilben just pushed the modifications to fix that. I changed window.navigator.userAgentData to window.navigator.userAgent which is supported on every browser.

MZN docs to Navitor.userAgent

jcfilben

This looks good to me, tested on Chrome, Safari, and FireFox on Mac

jcfilben · 2022-10-19T16:27:23Z

@MikeKingdom tagging you for review since you have a windows pc and I'm thinking it would be good to check this on different operating systems

MikeKingdom

I'll check it on windows shortly but in the meantime one small comment...

src/js/components/FileInput/FileInput.js

MikeKingdom

I tested this on windows and it is correct.

However I must note that windows' file explorer uses units of KB, MB etc even though it's technically KiB or MiB. If you're wanting the numbers to match they still won't because windows shows the Size detail column always in KB. The numbers won't match between the maxsize message and what windows shows in the detail column because windows will still show sizes in KB even if larger than 1024 KB. For example, windows file explore will show a file as 29,820 KB instead of 29.1 MB, In the property dialog for the file it will show "29.1 MB (30,535,680 bytes)". It's still not saying MiB but it will be technically MiB.

Isaac-alencar · 2022-10-21T13:53:27Z

@MikeKingdom Hey, thanks for reviewing this again. I've searching about that issue you related on windows, so I found this:

"This cannot be changed. Windows Explorer will display the size column in the smallest unit, “KB” for simplicity. If you select your file and view the Details Pane, you will see the file in its true form, whether it’s MB or GB."
Quora

Said that, do you think is a good approach create a business rule on the code only for windows conversion?

MikeKingdom · 2022-10-21T18:23:19Z

Said that, do you think is a good approach create a business rule on the code only for windows conversion?

If the goal is to make it was easier to know if your file is too big by looking at the file explorer details column (which is what you'll see when you click Browse to choose the file assuming that file dialog is set to the "details" view) then I'd think for windows it should show in KiB (the 1024 version)

Isaac-alencar · 2022-10-25T14:09:52Z

@MikeKingdom I've done some changes, so when you have a chance, could you review?

MikeKingdom

Thanks Isaac. I think it needs a couple tweaks on what it does for windows

MikeKingdom · 2022-10-25T15:52:50Z

src/js/components/FileInput/utils/formatBytes.js

+  const num = Math.ceil(size / IEC_CONVERSION_FACTOR);
+
+  function formatNumber(number) {
+    return number.toString().replace(/(\d)(?=(\d{3})+(?!\d))/g, '$1.');


Is this just trying to format the number with a delimiter every 1000? To do this correctly for the browser locale instead do:

Intl.NumberFormat().format(number)

Note that in en-US this uses a comma as the delimiter instead of period.

MikeKingdom · 2022-10-25T16:02:31Z

src/js/components/FileInput/utils/formatBytes.js

+    return number.toString().replace(/(\d)(?=(\d{3})+(?!\d))/g, '$1.');
+  }
+
+  return `${formatNumber(num)} ${'KB'}`;


Don't need the ${}:

return `${formatNumber(num)} KB`;

src/js/components/FileInput/utils/formatBytes.js

Isaac-alencar · 2022-10-27T14:41:07Z

@MikeKingdom I send some updates!

MikeKingdom · 2022-10-27T15:15:27Z

src/js/components/FileInput/utils/formatBytes.js

+  function formatNumber(number) {
+    return Intl.NumberFormat().format(number);
+  }
+
+  return `${formatNumber(num)} KB`;


Looks good and thanks for making the change. It might be nice to simplify this to

return `${Intl.NumberFormat().format(num)} KB`;

since the formatNumber function is trivial and not used elsewhere.

Isaac-alencar · 2022-10-31T18:25:07Z

Hi @MikeKingdom could you take a look again? I sent some updates 😃

MikeKingdom · 2022-10-31T18:34:14Z

src/js/components/FileInput/utils/formatBytes.js

+const windowsFormat = (size) => {
+  const num = Math.ceil(size / IEC_CONVERSION_FACTOR);
+
+  return Intl.NumberFormat().format(num);


Didn't we lose the units here? Shouldn't it be

return `${Intl.NumberFormat().format(num)} KiB`;

or it wouldn't bother me to say KB but KiB is techinically correct.

yup, I'll change to

return ${Intl.NumberFormat().format(num)} KB;

It is incorrect (technically speaking), but windows don't show to it's users with KiB

Hey, @MikeKingdom just did the updates!

userAgentData ins't compatible with all browsers

MikeKingdom

Looks good. Thanks!

taysea · 2022-11-03T20:44:43Z

src/js/components/FileInput/utils/formatBytes.js

+
+const getCurrentOS = () => {
+  const currentOS = ['Win', 'Linux', 'Mac'].find(
+    (v) => window.navigator.userAgent.indexOf(v) >= 0,


In the HPE Design System site, I'm getting an error when I try to run locally (with latest Grommet stable) of window is not defined. Think we might need to add some fallback for the case that window isn't defined yet?

Thanks to share it!. I'll work on it 👍🏻

Looks like a PR for this has already been filed #6480 to fix this. Appreciate the willingness to work on this though!

jcfilben reviewed Oct 18, 2022

View reviewed changes

src/js/components/FileInput/FileInput.js Outdated Show resolved Hide resolved

src/js/components/FileInput/FileInput.js Outdated Show resolved Hide resolved

jcfilben approved these changes Oct 19, 2022

View reviewed changes

jcfilben requested a review from MikeKingdom October 19, 2022 16:27

MikeKingdom reviewed Oct 19, 2022

View reviewed changes

src/js/components/FileInput/FileInput.js Outdated Show resolved Hide resolved

MikeKingdom reviewed Oct 19, 2022

View reviewed changes

MikeKingdom reviewed Oct 25, 2022

View reviewed changes

MikeKingdom reviewed Oct 27, 2022

View reviewed changes

Isaac-alencar force-pushed the feat/improve_fileinput_format_bytes branch from d15af4f to 0b990fd Compare October 28, 2022 00:35

MikeKingdom reviewed Oct 31, 2022

View reviewed changes

Isaac-alencar added 4 commits November 1, 2022 09:41

improve formatBytes function

c6e559c

change userAgenData to userAgent

a76747e

userAgentData ins't compatible with all browsers

refactor formatBytes function

1c469ac

small fixes on windowsFormat function

6276c98

Isaac-alencar force-pushed the feat/improve_fileinput_format_bytes branch from 0b990fd to 6276c98 Compare November 1, 2022 12:55

MikeKingdom approved these changes Nov 1, 2022

View reviewed changes

jcfilben requested a review from ericsoderberghp November 2, 2022 18:39

ericsoderberghp added 2 commits November 2, 2022 11:43

Merge branch 'master' into feat/improve_fileinput_format_bytes

ee54a4f

Merge branch 'master' into feat/improve_fileinput_format_bytes

cad117c

ericsoderberghp approved these changes Nov 2, 2022

View reviewed changes

ericsoderberghp merged commit 92edbea into grommet:master Nov 2, 2022

taysea reviewed Nov 3, 2022

View reviewed changes

taysea mentioned this pull request Nov 3, 2022

Add optional chaining to formatBytes #6480

Merged

4 tasks

Feat: improve formatBytes function #6427

Feat: improve formatBytes function #6427

Conversation

Isaac-alencar commented Oct 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Where should the reviewer start?

What testing has been done on this PR?

How should this be manually tested?

Do Jest tests follow these best practices?

Any background context you want to provide?

What are the relevant issues?

Screenshots (if appropriate)

Do the grommet docs need to be updated?

Should this PR be mentioned in the release notes?

Is this change backwards compatible or is it a breaking change?

Uh oh!

jcfilben left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Isaac-alencar commented Oct 19, 2022

Uh oh!

Isaac-alencar commented Oct 19, 2022

Uh oh!

jcfilben left a comment

Choose a reason for hiding this comment

Uh oh!

jcfilben commented Oct 19, 2022

Uh oh!

MikeKingdom left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MikeKingdom left a comment

Choose a reason for hiding this comment

Uh oh!

Isaac-alencar commented Oct 21, 2022

Uh oh!

MikeKingdom commented Oct 21, 2022

Uh oh!

Isaac-alencar commented Oct 25, 2022

Uh oh!

MikeKingdom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Isaac-alencar commented Oct 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Isaac-alencar commented Oct 31, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MikeKingdom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Isaac-alencar commented Oct 14, 2022 •

edited

Loading