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.

Sign up for GitHub

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

Jump to bottom

Column placeholder HLD #1869

Merged
merged 21 commits into from
Mar 7, 2024
Merged

Column placeholder HLD #1869

merged 21 commits into from
Mar 7, 2024

Conversation

mollykreis
Copy link
Contributor

Pull Request

🤨 Rationale

Create HLD for column placeholders in the table.

👩‍💻 Implementation

N/A

🧪 Testing

N/A

✅ Checklist

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

@mollykreis
Copy link
Contributor Author

@rajsite and @fredvisser - can you take a look at this draft HLD and make sure it aligns with your expectations after our discussion last week?

@mollykreis mollykreis mentioned this pull request Feb 27, 2024
Merged
1 task
mollykreis added a commit that referenced this pull request Feb 29, 2024
@mollykreis @rajsite
Empty icon mapping (#1874)
d4ca92e 
# Pull Request

## 🤨 Rationale

Grouping on an icon value isn't ideal if any of the cells don't have an
icon. Specifically, the group row doesn't have a value associated with
it and renders only a row count.

It would be nice if we could allow a client to leave a cell blank for
certain field values but still have the group rows have a value
associated with them. For example, have a column that shows either a
spinner or nothing based on whether a task is in progress. When
grouping, it would be nice to show a string, such as "Idle", associated
with the records that have nothing in progress without cluttering the
table with extra icons in cells.

This is the Future Work described in #1869

## 👩‍💻 Implementation

Updated the icon column to allow a `nimble-mapping-icon` to have an
`undefined` icon. In that case, the cell renders nothing but the group
row renders only the text associated with the mapping.

## 🧪 Testing

Manually tested in storybook.
Added new unit tests for having an `undefined` icon
Updated storybook/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). -->

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

---------

Co-authored-by: Milan Raj <[email protected]>
@mollykreis mollykreis requested a review from rajsite March 1, 2024 20:45
@mollykreis mollykreis marked this pull request as ready for review March 1, 2024 20:45
@fredvisser fredvisser requested a review from TJ-G March 4, 2024 20:02
@mollykreis mollykreis enabled auto-merge (squash) March 7, 2024 21:44
@mollykreis mollykreis merged commit ece50ad into main Mar 7, 2024
9 checks passed
@mollykreis mollykreis deleted the column-placeholder-hld branch March 7, 2024 22:00
@mollykreis mollykreis mentioned this pull request Mar 20, 2024
Merged
1 task
mollykreis added a commit that referenced this pull request Mar 27, 2024
@mollykreis @rajsite
Add cell placeholders (#1950)
a01932b 
# Pull Request

## 🤨 Rationale

This PR adds a `placeholder` configuration to the following table
columns as described in #1869:
- anchor
- date-text
- duration-text
- number-text
- text

This is part of #1538. The remaining work is to add the `placeholder`
attributes to the Angular and Blazor wrappers for each column.

As part of this PR, I also included best practices established for each
column in the storybook docs. These are adapted from the placeholder
HLD.

## 👩‍💻 Implementation

I implemented this change in a similar way as #1914. Specifically, I
updated `TableColumnTextCellViewBase` to have its own implementation of
`columnConfigChanged` and `cellRecordChanged` that calls a new
`applyPlaceholderTextIfNeeded` function. This required me to create a
`TableColumnTextBaseCellRecord` interface to enforce that the cell
records used by `TableColumnTextCellViewBase` have a `value` property
and to create a `TableColumnTextBaseColumnConfig` interface to enforce
that the column configs used by `TableColumnTextCellViewBase` have an
optional `placeholder` property.

## 🧪 Testing

- Updated/created unit tests
- Manually tested in storybook
- Updated matrix tests to test each relevant column with & without a
placeholder

## ✅ 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: Milan Raj <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vpstoynova vpstoynova vpstoynova left review comments

@fredvisser fredvisser fredvisser approved these changes

@jattasNI jattasNI jattasNI approved these changes

@rajsite rajsite rajsite approved these changes

@atmgrifter00 atmgrifter00 Awaiting requested review from atmgrifter00 atmgrifter00 is a code owner

@TJ-G TJ-G Awaiting requested review from TJ-G

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mollykreis @fredvisser @rajsite @jattasNI @vpstoynova