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

Table allows configuration of number column type #1011

Closed
16 tasks done
nate-ni opened this issue Jan 30, 2023 · 2 comments
Closed
16 tasks done

Table allows configuration of number column type #1011

nate-ni opened this issue Jan 30, 2023 · 2 comments
Assignees
Labels
enhancement New feature or request

Comments

@nate-ni
Copy link
Contributor

nate-ni commented Jan 30, 2023

😯 Problem to Solve

A user needs to display numeric values to the user. These values may be integer or decimal based numbers.

The end user would like to be able to sort, filter, etc. based on these values.
Note: This feature would not provide that direct functionality but is a consideration for this feature.

💁 Proposed Solution

The user should be able to provide simple, supported, configurations easily.

Open Questions

Question Initial Recommendation Resolved Decision
Which formatting options do we need to provide? Integer and Decimal No
Should we provide config for prefix? (e.g. currency symbols) No No
Should we provide config for prefix or suffix? (e.g. static string for units) Yes No
How should we provide alignment? Always right-align content No
Which width modes should we support? Both fixed pixel width and fractional width No

Other Considerations

Supporting the users local for number formatting
More info: Intl.NumberFormat - JavaScript | MDN

References/Prior Art

jQuery Grid Cells Formatting

📋 Tasks

Preview Give feedback
  1. UX-Deprecated
    RickA-NI
  2. enhancement needs investigation
    m-akinc
@nate-ni nate-ni added enhancement New feature or request triage New issue that needs to be reviewed labels Jan 30, 2023
@nate-ni nate-ni added this to the Table Milestone 2 milestone Jan 30, 2023
@jattasNI
Copy link
Contributor

I was curious about what kind of numeric formatting options we need. Here are the use cases I found in SLE that use either a number column type or a custom format function:

  1. data table grid includes integer counts with default formatting
  2. executions grid converts a numeric enum to a user-visible string. I think we'd rather make this a dedicated enum column type
  3. test results grid formats elapsed time as "12 s" or "13 m, 12 s". I think we'd rather make this a dedicated elapsed time column type, possibly leveraging upcoming browser APIs like temporal
  4. files grid formats file sizes using units like bytes, KB, MB, etc. I think we'd make this a dedicated custom column type.
  5. wafer classification has lots of numeric columns but I believe they're all integer counts

Other use cases include formatting dates, times, icons, arbitrary custom columns, etc are covered by other work items.

Initial conclusion: It might be possible to keep the number column type extremely simple to start with by using default numeric formatting and not allowing units. Posting this to capture notes and hear hot takes, but we can hash out in detail in an HLD.

@jattasNI
Copy link
Contributor

The SLS Tag grid shows tag values with numeric formatting that is not end-user configurable. Some of these use cases also might require a custom column because tags can be various non-numeric data types.

image

@jattasNI jattasNI removed the triage New issue that needs to be reviewed label Jan 31, 2023
@nate-ni nate-ni moved this from Backlog to Defined/Ready to Pickup in Nimble Design System Priorities Feb 1, 2023
@jattasNI jattasNI moved this from Defined/Ready to Pickup to In progress in Nimble Design System Priorities Feb 13, 2023
jattasNI added a commit that referenced this issue May 11, 2023
)

# Pull Request

## 🤨 Rationale

Start the HLD for #1011 and other column types that display data as
text. This update focuses on shared base classes, client-specified
custom colums, and possible elements that Nimble could define, not APIs
for specific elements.

We previously brainstormed this in #1054 but I started a new PR for this
more concrete proposal.

## 👩‍💻 Implementation

This includes the HLD and also some updates to component naming
conventions which fell out of the brainstorming.

## 🧪 Testing

N/A

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [x] I have updated the project documentation to reflect my changes or
determined no changes are needed.

---------

Co-authored-by: mollykreis <[email protected]>
Co-authored-by: Milan Raj <[email protected]>
rajsite added a commit that referenced this issue May 12, 2023
…1239)

# Pull Request

## 🤨 Rationale

As part of the vision described in #1224 in service of #1011 and other
column types, we will want to share rendering logic across columns that
display their data as text. This could eventually include:
 - the existing text column
 - the mapping column discussed in #1220 
 - columns that display numbers, dates, elapsed time, and more
 - columns defined in applications with custom formatting logic

## 👩‍💻 Implementation

1. Extract abstract base classes, templates, and styles from the
existing text column for:
    - the column custom element
    - the cell view custom element
    - the group header custom element
These live in `table-column/text-base`.
2. Derive concrete instances and register custom elements of each of
these for the `text` column
3. Define and use some table field types to provide stricter typing some
generic parameters

## 🧪 Testing

Relying on existing tests. 

We have a future task to add docs/examples/tests for applications
deriving from these base classes.

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [x] I have updated the project documentation to reflect my changes or
determined no changes are needed.

---------

Co-authored-by: Milan Raj <[email protected]>
jattasNI added a commit that referenced this issue Jun 15, 2023
# Pull Request

## 🤨 Rationale

Need APIs for #1011 and #1014. Builds on #1224.

## 👩‍💻 Implementation

Updates to existing HLD. Philosophy is to rely on the browser's `Intl`
formatting APIs when possible and to provide modes with opinionanted
configuration and with custom configuration.


## 🧪 Testing

N/A

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [X] I have updated the project documentation to reflect my changes or
determined no changes are needed.
mollykreis added a commit that referenced this issue Aug 17, 2023
# Pull Request

## 🤨 Rationale

This is part of #1011 

This PR adds the initial implementation of the
`nimble-table-column-number-text` with configuration to render the
number using a default formatter or as an integer.

There are a number of additional configurations that still need to be
added to the column, such as:
- Text alignment (right vs left)
- Additional formatting options (decimal and custom)
- Prefix & suffix
- Locale-aware formatting

## 👩‍💻 Implementation

The new column follows the same patterns as other table columns in
nimble and uses the existing `TableColumnTextCellViewBase` and
`TableColumnTextGroupHeaderViewBase`.

More interesting/complex implementation will come in future PRs.

## 🧪 Testing

- New unit tests
- New storybook page (manually tested)
- New matrix tests

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [x] I have updated the project documentation to reflect my changes or
determined no changes are needed.

---------

Co-authored-by: Malcolm Smith <[email protected]>
Co-authored-by: Jonathan Meyer <[email protected]>
mollykreis added a commit that referenced this issue Aug 24, 2023
# Pull Request

## 🤨 Rationale

This is part of #1011. It adds an `alignment` attribute to the
`nimble-table-column-number-text` element that can be set to `right`,
`left`, or `default` (defined in a new `NumberTextAlignment` enum). When
`alignment` is set to `default`, the alignment is determined by the
column's `format` property.

## 👩‍💻 Implementation

- Added `alignment` property to the base text cell view that defaults to
`left` so that the base text cell view can control the alignment
- Added new `NumberTextAlignment` enum with values of `right`, `left`,
and `default`
- Added `alignment` attribute & property to the
`nimble-table-column-number-text`

## 🧪 Testing

- New unit tests
- Extended storybook story and manually verified the changes look
correct
- Added additional matrix tests to cover each `format` option being
configured with each `alignment` option

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

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

---------

Co-authored-by: m-akinc <[email protected]>
Co-authored-by: Jesse Attas <[email protected]>
mollykreis added a commit that referenced this issue Aug 28, 2023
# Pull Request

## 🤨 Rationale

This PR is part of #1011. It adds support for making a table column have
`decimal` formatting, which includes:

- Adding a new `format` value of `decimal`
- Adding `decimal-digits` attribute
- Adding validation that `decimal-digits` is in the allowed range
- Making the default alignment of a `decimal` column `right`

## 👩‍💻 Implementation

The general approach of these new capabilities is aligned with other
columns that have been implemented. A few notes that might be of
interest:

- This PR adds a `TableColumnNumberTextValidator` to validate the
`decimal-digits`. The column is invalid if its `format` is `decimal` and
`decimal-digits` is outside of the supported range.
- Added a new `DecimalFormatter` class that extends `NumberFormatter`.
It differs slightly from the `RoundToIntegerFormatter` and
`DecimalFormatter` in that the `formatter` is not static because the
number of decimals being rendered can vary between columns.

## 🧪 Testing

- New unit tests
- Added additional functionality to storybook story & manually verified
it was correct
- Added another column to the matrix tests to show the decimal format

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [ ] I have updated the project documentation to reflect my changes or
determined no changes are needed.
rajsite pushed a commit that referenced this issue Aug 31, 2023
# Pull Request

## 🤨 Rationale

This is part of #1011.

It adds Angular support for the number-text column.

## 👩‍💻 Implementation

This PR adds Angular support for the number-text column. The column
isn't complete yet in `nimble-components`, but this adds Angular support
for what is implemented so far.

## 🧪 Testing

- Verified the column works as expected in the Angular example app.
- Unit tests for the directive.
- Used locally built package to verify the column can be leveraged in
SLE

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

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

---------

Co-authored-by: m-akinc <[email protected]>
rajsite pushed a commit that referenced this issue Sep 12, 2023
# Pull Request

## 🤨 Rationale

This is part of #1011 

This PR adds support for the number-text column using the `lang` token
on the theme provider to format numbers in a locale-aware way.

## 👩‍💻 Implementation

I followed the same pattern used in the date-text column to listen for
changes to the `lang` theme provider token and pass that into the
formatters for the number-text column.

## 🧪 Testing

- Manually tested in storybook
- New unit test for the column that verifies the formatter is updated
with the `lang` changes
- New unit tests for each formatter that verify the locale is used when
formatting
- Note: I updated the formatter tests to run all the test cases with an
additional locale. On one hand this potentially seemed like "too many"
locale tests, but I thought it was the best way to ensure that the
locale was used in all internal formatters (e.g. the `default` formatter
uses 3 formatters internally) and that we had coverage for a wide range
of possible values to be formatted.

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [ ] I have updated the project documentation to reflect my changes or
determined no changes are needed.
mollykreis added a commit that referenced this issue Sep 14, 2023
# Pull Request

## 🤨 Rationale

This is part of #1011 

The Angular directive already exists for the number-text column, but it
is missing `alignment` because that property was added after the
directive was created initially.

## 👩‍💻 Implementation

Add `alignment` to the directive along with all the standard tests

## 🧪 Testing

Auto tested

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

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

---------

Co-authored-by: m-akinc <[email protected]>
rajsite pushed a commit that referenced this issue Sep 15, 2023
# Pull Request

## 🤨 Rationale

This adds Blazor support for the number-text column, which is part of
#1011

## 👩‍💻 Implementation

- Add Blazor component and tests for the number-text column following
the pattern we've used for other table columns
- Add number-text column to the example Blazor app

## 🧪 Testing

- Added new unit tests
- Manually verified that the example app behaves as expected

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [ ] I have updated the project documentation to reflect my changes or
determined no changes are needed.
@mollykreis mollykreis moved this from In progress to Pending in Nimble Design System Priorities Sep 15, 2023
@jattasNI jattasNI moved this from Pending to Done in Nimble Design System Priorities Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Development

No branches or pull requests

5 participants