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

File size table column spec #1566

Closed
wants to merge 11 commits into from
Closed

Conversation

m-akinc
Copy link
Contributor

@m-akinc m-akinc commented Sep 25, 2023

Pull Request

🀨 Rationale

Spec for file size table column

πŸ‘©β€πŸ’» Implementation

N/A

πŸ§ͺ Testing

N/A

βœ… Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

@m-akinc
Copy link
Contributor Author

m-akinc commented Sep 25, 2023

@atmgrifter00 could you buddy this please? I guess it's more of a "preliminary" review, since you are a required reviewer anyway.

`nimble-table-column-file-size`

- `field-name` - name of the record field containing the data. Data must be a `number` byte count.
- `unit-type` - "binary" (KiB, MiB, GiB, TiB, PiB) or "decimal" (KB, MB, GB, TB, PB); defaults to "decimal"
Copy link
Contributor

Choose a reason for hiding this comment

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

What's influencing our decision to default to decimal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

KB/MB/GB seem much more prevalent KiB/MiB/GiB. For example, all of the prior art examples I gave, plus some below use the decimal-type units:
GitHub
image
Gmail
image
npmjs.com
image

I couldn't find any examples of KiB/MiB/GiB being used in the wild. Granted, it's not clear if every example I found was actually using a 1000 conversion factor, but I don't have a reason believe otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

I looked around for an authoritative source for guidance. This article is a good overview. Some highlights:

  • There are standards which say "don't use decimal prefixes (e.g. K) for binary values"
  • There are lots of examples which violate those standards (e.g. Windows)
  • I haven't found a standard which says whether to prefer binary or decimal. It still seems to be context-specific

That said, I'd also vote for decimal as the default. I don't have strong rationale other than that the prefixes are older and probably more familiar to users.

Copy link
Contributor

Choose a reason for hiding this comment

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

As a point of reference, it seems like Windows explorer (which shows units as KB, MB, GB), uses a conversion factor of 1024 (although sometimes the math really looks like they are using 1025, which is a bit perplexing).

In this example from the "Properties" window for a file, you can see that the KB is not the bytes divided by 1000:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not as straightforward as I thought, as it seems that 1024 is a better default conversion for file sizes, even though most places incorrectly use the decimal units (KB, etc).


### Visual Appearance

Will use same text styling as the `nimble-table-column-text`.
Copy link
Contributor

@atmgrifter00 atmgrifter00 Sep 26, 2023

Choose a reason for hiding this comment

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

Despite the presence of a unit, this column still feels more like a numeric text column in spirit. One thing I could imagine a client wanting to do is have all of the values right aligned, and possibly having them all have the same amount of fractional digits.

Ultimately, it feels like we're missing some design decisions that might dictate some necessary APIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that fundamentally, we're displaying a numeric value, but I don't think that necessarily brings along any additional requirements compared to a text column (assuming that both are read-only).

I've added a justification for left-aligning values in the Visual Appearance section. That argument hinges on us not using a consistent number of decimal digits and not using a consistent unit. If we think it is worth providing those options, then right-alignment could make sense. I don't think we currently have any clients asking for that, but there's nothing keeping us from adding that support at a later time.

Let's see what the rest of the team thinks.

Copy link
Contributor

@atmgrifter00 atmgrifter00 Sep 27, 2023

Choose a reason for hiding this comment

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

I'll just note that in all of the prior art you provided, save SLE, columns displaying file size were right-aligned :). This may raise a question of whether or not we should consider that a "user expectation". Ultimately, I agree, that these are features that can be added over time. Given that, I think it might make sense to word your explanation for doing left-alignment a bit differently, in that we shouldn't say this column won't do that, but that if we decide that we should allow users this flexibility we can add those features in a non-breaking way. Maybe there should be a 'Future Considerations' section?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @m-akinc -- the rationale we were given by visual designers was that right-aligned values made sense if all values had the same number of decimals and the same length of units.

Perhaps, this is really a question for the visual designers to see what their expectation is for this column's alignment.

@m-akinc m-akinc marked this pull request as ready for review September 27, 2023 14:00

### Examples

Given a data value of 2856 (bytes):
Copy link
Contributor

@atmgrifter00 atmgrifter00 Sep 27, 2023

Choose a reason for hiding this comment

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

nit: The grokking of what is going on here might be helped if the value was simply 1024 (or some integer multiple of that).


**Cons:** Less convenient for clients. Does not honor the `lang` setting on the page or on `nimble-theme-provider`.

I suggest we go with option 1, primarily because I am hesitant to add twelve new label provider strings that clients are expected to localize. I suggest initially supporting the following languages:
Copy link
Contributor

Choose a reason for hiding this comment

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

Another option to consider is to use Intl.NumberFormatter as described by this SO answer. It can handle conversion (only base 1000, not 1024) and has units for every language built in.

I'm kind of tempted to leverage this because it solves localization, even at the expense of supporting binary conversions.

You could also do some googling and see if there's a roadmap or polyfills that support binary conversions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't believe I missed that. However, after just a little experimentation, I'm pretty unhappy with some of the formatting and translations.
English:
image

  • No space betwen number and units
  • "B" instead of "byte" or "bytes"
  • "BB" instead of "GB"

French:
image

  • inconsistent spacing between number and units
  • "o" instead of "octet" or "octets"
  • "Mdo" instead of "Go"
  • "Bno" instead of "To"

German:
image

  • these all seem weird, especially the lack of KB

Spanish
image

  • no GB, and others seem questionable

I could go on, but in general, I have a lot of doubts about letting Intl.NumberFormat handle the unit translation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that the abbreviation for bytes seems inferior to spelling it out. I also agree that the translations seem a little weird, but it might be worth checking with localization to see if they're actually sane. I would assume the browsers are following some standard which was derived by experts who know about common usage in each language, so just because it looks weird to us doesn't mean it's wrong.

One other angle to consider comes from another SO answer. We could use the translated unit strings from the number formatter while doing the actual conversions ourselves. i.e. once we determine the unit should be GB then we explicitly ask the formatter to give us a value that uses GB. But one gap I see with that approach is that it doesn't seem to handle pluralizing "bytes".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added options 3 and 4 that are variations on using Intl.NumberFormat just for unit translation. Both require giving up support for the KiB-type unit labels in one way or another.

Because of the cons of the Intl.NumberFormat approaches, I'm now advocating for option 2. I'm waiting to hear others' votes, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also liking option 2 best right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

One downside of option 2 is that we still don't have a good story for localizing Nimble strings in Blazor apps. That's been less noticeable for tooltips and accessible labels but might become more noticeable with visible column text.

Copy link
Member

Choose a reason for hiding this comment

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

Gah... I think I'm leaning towards option 1...

Option 2 pushes a fair bit of integration work on everyone who tries to use the column. How much benefit are they really getting from the column if they have to juggle and possibly mess-up or be inconsistent about digital vs decimal translations across languages. Seems extra unsatisfying work. I can imagine wanting to use a theme-provider to override labels for different increment / decrement buttons within a page, but I can't really imagine a scenario where that flexibility is good here.

And it's a bummer that the standards groups are pushing back on the EIC formats of mebibytes, etc from following these threads: tc39/ecma402#32 (comment)

I think we should support those for the accuracy.. I don't think anyone would actually be completely lost by us using KiB and MiB for digital. Think they'll figure it out and maybe learn something. Would be nice to be able to help build some consistency for units at a T&M company, maybe this can be our push for that :) (related wonder if the NI brand guidelines cover any of this)

Copy link
Member

Choose a reason for hiding this comment

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

Devtools research on trying to be consistent: https://goo.gle/devtools-si

and the relevant xkcd lol
image

Copy link
Member

Choose a reason for hiding this comment

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

Another meme, this one from reddit (we did the full curve!):
image

The comments are interesting: https://www.reddit.com/r/ProgrammerHumor/comments/17r2bqx/aretheystupid/

Someone from Azure discussed there approach for defining column units in a table: https://www.reddit.com/r/ProgrammerHumor/comments/17r2bqx/aretheystupid/k8h2fw7/?context=10000

Direct link to Azure column handling docs: https://github.com/Azure/portaldocs/blob/main/portal-sdk/generated/declarative-assets.md#custom-columns


### Visual Appearance

Will use same text styling as the `nimble-table-column-text`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @m-akinc -- the rationale we were given by visual designers was that right-aligned values made sense if all values had the same number of decimals and the same length of units.

Perhaps, this is really a question for the visual designers to see what their expectation is for this column's alignment.


### Risks and Challenges

The byte formatting pipe currently used in SLE converts using a factor of 1024, but uses unit labels KB/MB/GB. In order to avoid showing different values or units for the same file outside of an `sl-table`, we should modify the SLE pipe to be consistent with our implementation. If there are other clients that would have the same problem, they will just display the file sizes inconsistently.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we export a formatting function that clients can call for values outside their tables to guarantee the format is identical?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems a little strange to be turning to your design system to get a function for file size formatting, but maybe? If we do, it should be exported from utilities and not from anything table-related. I feel like it might be more natural for clients to provide us a formatting function (to override our default). I'm not sure it's worth providing the support, though.

`nimble-table-column-file-size`

- `field-name` - name of the record field containing the data. Data must be a `number` byte count.
- `unit-type` - "binary" (KiB, MiB, GiB, TiB, PiB) or "decimal" (KB, MB, GB, TB, PB); defaults to "decimal"
Copy link
Contributor

Choose a reason for hiding this comment

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

I looked around for an authoritative source for guidance. This article is a good overview. Some highlights:

  • There are standards which say "don't use decimal prefixes (e.g. K) for binary values"
  • There are lots of examples which violate those standards (e.g. Windows)
  • I haven't found a standard which says whether to prefer binary or decimal. It still seems to be context-specific

That said, I'd also vote for decimal as the default. I don't have strong rationale other than that the prefixes are older and probably more familiar to users.


---

## Open Issues
Copy link
Contributor

Choose a reason for hiding this comment

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

One alternative that we should consider: a single column type that supports displaying a numeric value and a unit for a fixed set of units. To start with it would be used for file size and elapsed time, but eventually someone could add support for V, degrees C, etc.

A possible API would be a single attribute like unit = 'byte-binary' | 'byte-decimal' | 'elapsed-time' | 'volt' | ...

Or maybe unit = 'byte' | 'second' | 'volt' and byte-format = 'binary' | 'decimal' and second-format = 'h:m:s' | 'h,m,s'.

Maybe worth talking about in person with the team to move the discussion along.


**Cons:** Less convenient for clients. Does not honor the `lang` setting on the page or on `nimble-theme-provider`.

I suggest we go with option 1, primarily because I am hesitant to add twelve new label provider strings that clients are expected to localize. I suggest initially supporting the following languages:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also liking option 2 best right now.

- correct units used for both `unit-type`s
- standard cell text/group header text ellipsizing tests
- honors `lang` value and responds to changes to `lang` value
- uses English labels when unsupported locale is given
Copy link
Contributor

Choose a reason for hiding this comment

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

As a note, I think this "uses English labels when unsupported locale is given" is dependent on what locale approach we take. That might not end up being the behavior we expect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll make sure to update it if we decide on a different approach.


- Is there a better name for this column type? E.g. `nimble-table-column-memory-size`.
- Should we support grouping?
- What is our unit localization approach?
Copy link
Contributor

Choose a reason for hiding this comment

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

I think another open question related to unit localization is what the expected/acceptable behavior is from our POs. Some of the tradeoffs for localization have to do with showing MB rather than MiB, and we probably need someone to decide if that's an acceptable tradeoff to make.

@@ -0,0 +1,7 @@
{
Copy link
Member

@rajsite rajsite Oct 3, 2023

Choose a reason for hiding this comment

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

Think there are open questions @fredvisser and @jattasNI were following up on:

  1. PO requirements on if both decimal vs binary display of byte sizes, do we need both or can we standardize on one?
  2. If showing binary display, can we use the NIST / ISO / ANSI binary prefixes, i.e. MiB, GiB and migrate all usages in SystemLink to be standardized (other pipes, etc).
  3. Maybe explicitly, can we avoid using MB, GB in the ambiguous / legally contentious way altogether.
  4. Do we have tech writing / ni.com / other guidance on decimal vs binary sizing we can align on to drive consistency across all applications?

Mert is OOO this week and with the open questions I think we are blocked on reviews for now so moving back to draft. Feel free to re-open if there are parts to review ATM.

@rajsite rajsite marked this pull request as draft October 3, 2023 21:32
@jattasNI jattasNI mentioned this pull request Oct 3, 2023
@m-akinc
Copy link
Contributor Author

m-akinc commented Oct 17, 2023

Closing. We're changing direction to extend the functionality of the number-text column instead of creating a separate column.

@m-akinc m-akinc closed this Oct 17, 2023
@m-akinc m-akinc deleted the users/makinc/file-size-column-hld branch December 1, 2023 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants