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

HLD brainstorming - Formatted text table columns #1054

Closed
wants to merge 31 commits into from

Conversation

jattasNI
Copy link
Contributor

@jattasNI jattasNI commented Feb 13, 2023

Pull Request

🀨 Rationale

First pass of design document for #1011. I'm considering formatted text more holistically, so this is also relevant to #1014.

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

The document describes several alternative high-level approaches and proposes which ones we should pursue. Once we agree on the high-level direction in this PR, I will move it back to draft while I refine the proposed APIs and fill in the rest of the HLD template.

@jattasNI jattasNI changed the title HLD - Formatted numeric text columns HLD - Formatted text columns Feb 13, 2023

For the sake of discussion my initial proposal is:

1. For columns that require app-specific formatting logic I'm leaning towards "Client specifies formatting function" over "Use `table-column-text`" because it seems more like the API that app developers would expect (perhaps I'm biased by previous implementations). I'd like to do performance profiling to see how it impacts scroll performance before committing to this direction.
Copy link
Contributor

@atmgrifter00 atmgrifter00 Feb 14, 2023

Choose a reason for hiding this comment

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

I think the possible proposed solution outlined in the "Client specifies formatting function" section is at odds with what an app developer would expect for an API, in that they would be expected to create a custom column implementation. The first variant, I think, is what clients typically expect, and resembles what Ag-grid provides.

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 leave this open since I haven't addressed it (it's one level more detailed than I'm seeking agreement on at this moment, though still a good discussion to have).

I like the proposed approach of deriving from an abstract element class to define a new column type because it makes it easy to associate the formatting function with the column definition in JS code (they're in the same class). The approach of setting a formatting function via a property on the column element requires you to get a reference to the column in JS/TS/C# code which seemed more annoying to me (especially in Blazor).

@jattasNI jattasNI marked this pull request as ready for review March 2, 2023 21:23
@jattasNI jattasNI requested a review from rajsite as a code owner March 2, 2023 21:23
field-name="progress"
locales="en-US"
style="percent"
minimum-fraction-digits=2
Copy link
Contributor

@atmgrifter00 atmgrifter00 Mar 3, 2023

Choose a reason for hiding this comment

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

One thing that may be worth considering is if we do have a nimble-table-column-number that exposes an API for formatting the value, whether or not that API should be reflected in the number-field component (and what the implications of that are).

@jattasNI jattasNI marked this pull request as draft April 7, 2023 20:42
@jattasNI jattasNI changed the title HLD - Formatted text columns HLD brainstorming - Formatted text table columns May 3, 2023
@jattasNI
Copy link
Contributor Author

jattasNI commented May 3, 2023

Closing in favor of #1224. All of the excellent feedback in this PR has been captured in that update.

@jattasNI jattasNI closed this May 3, 2023
jattasNI added a commit that referenced this pull request 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]>
@jattasNI jattasNI deleted the number-column-hld branch December 4, 2023 14:56
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.

4 participants