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

Update mapping column HLD #1992

Merged
merged 22 commits into from
Apr 11, 2024
Merged

Update mapping column HLD #1992

merged 22 commits into from
Apr 11, 2024

Conversation

mollykreis
Copy link
Contributor

@mollykreis mollykreis commented Apr 1, 2024 •
edited
Loading

Pull Request

🤨 Rationale

As part of #1873, we've decided to make some changes to the mapping columns.

The changes are described in detail in the HLD, but the high-level changes are:

  • nimble-table-column-enum-text and nimble-table-column-icon will be replaced with a single column named nimble-table-column-mapping
  • mapping elements are reworked to support icon/spinner + text in the same cell
  • there will be a configuration on the column to make it fixed at 32px

👩‍💻 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

@m-akinc, will you buddy this PR for me?

@mollykreis mollykreis requested a review from m-akinc April 1, 2024 21:33
m-akinc
m-akinc reviewed Apr 2, 2024
View reviewed changes
Copy link
Contributor

@m-akinc m-akinc left a comment

Choose a reason for hiding this comment

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

My primary feedback is that I'd like us to revisit the API (and remove/deprecate the existing elements) rather sticking to names that don't make as much sense anymore.

packages/nimble-components/src/table/specs/table-column-specs/table-column-mapping.md Outdated Show resolved Hide resolved
packages/nimble-components/src/table/specs/table-column-specs/table-column-mapping.md Outdated Show resolved Hide resolved
packages/nimble-components/src/table/specs/table-column-specs/table-column-mapping.md Outdated Show resolved Hide resolved
packages/nimble-components/src/table/specs/table-column-specs/table-column-mapping.md Outdated Show resolved Hide resolved
packages/nimble-components/src/table/specs/table-column-specs/table-column-mapping.md Outdated Show resolved Hide resolved
packages/nimble-components/src/table/specs/table-column-specs/table-column-mapping.md Outdated Show resolved Hide resolved
packages/nimble-components/src/table/specs/table-column-specs/table-column-mapping.md Outdated Show resolved Hide resolved
packages/nimble-components/src/table/specs/table-column-specs/table-column-mapping.md Show resolved Hide resolved
packages/nimble-components/src/table/specs/table-column-specs/table-column-mapping.md Outdated Show resolved Hide resolved
packages/nimble-components/src/table/specs/table-column-specs/table-column-mapping.md Outdated Show resolved Hide resolved
@mollykreis mollykreis requested a review from m-akinc April 3, 2024 18:34
@mollykreis mollykreis marked this pull request as ready for review April 4, 2024 17:36
rajsite
rajsite approved these changes Apr 9, 2024
View reviewed changes
Copy link
Member

@rajsite rajsite left a comment

Choose a reason for hiding this comment

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

API-wise looks good, just some Qs on naming

packages/nimble-components/src/table/specs/table-column-specs/table-column-mapping.md Outdated Show resolved Hide resolved
@rajsite rajsite merged commit 559ede8 into main Apr 11, 2024
11 checks passed
@rajsite rajsite deleted the icon-column-hld branch April 11, 2024 15:58
@mollykreis mollykreis mentioned this pull request Apr 11, 2024
Merged
1 task
mollykreis added a commit that referenced this pull request Apr 17, 2024
@mollykreis
Add configuration for icon mappings and spinner mappings to show/hide…
6d50261 
… their text within a table's cells (#2012)

# Pull Request

## 🤨 Rationale

This updates the `nimble-mapping-icon` and `nimble-mapping-spinner`
elements to be aligned with the changes described in #1992.

## 👩‍💻 Implementation

- Added `text-hidden` to `nimble-mapping-icon` and
`nimble-mapping-spinner`
- I also added these to Blazor and Angular so that when the change is
published clients in any framework can uptake the new package and have
the opportunity to add `text-hidden` to mappings in order to keep their
existing behavior
- Reworked the `MappingIconConfig` and `MappingSpinnerConfig` to be more
aligned
- They both create the appropriate `ViewTemplate` to render a spinner,
icon, or blank
- They both add the appropriate `title` and `aria-*` attributes based on
the configuration
- Neither adds css classes that are styled in the group header styles or
cell view styles
- Update the templates for the icon column's cell view and group header
share code for rendering icons and spinners (enabled because of aligning
`MappingIconConfig` and `MappingSpinnerConfig`) and to render the text
based on the `text-hidden` attribute

## 🧪 Testing

- Updated storybook & matrix tests
- Manually tested
- Wrote/updated a number of tests for the icon column
- To enable this, created a page object for the icon column to
encapsulate the logic around finding the icon/spinner and the text

## ✅ 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 mentioned this pull request Apr 17, 2024
Merged
1 task
mollykreis added a commit that referenced this pull request Apr 18, 2024
@mollykreis
Support text mappings in icon column (#2023)
73a19a6 
# Pull Request

## 🤨 Rationale

This adds support to `nimble-table-column-icon`* for text mappings as
described in #1992.

*`nimble-table-column-icon` will be renamed to
`nimble-table-column-mapping` in a future PR

## 👩‍💻 Implementation

I've updated the column validator to allow text mappings and updated the
cell-view and group-header-view to render text mappings correctly.

## 🧪 Testing

- Updated storybook and matrix tests
- New unit 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.
@mollykreis mollykreis mentioned this pull request Apr 24, 2024
Merged
1 task
rajsite added a commit that referenced this pull request Apr 30, 2024
@mollykreis @atmgrifter00 @rajsite
Allow icon column to be configured as fixed width (#1907)
6b0f337 
# Pull Request

## 🤨 Rationale

This change is part of #1873 and follows the decisions made in #1992

This change adds the ability to configure the `width-mode` of the icon
column. When set to `iconSize`, the column is a fixed width of 32px,
cannot be resized, and hides its sorting & grouping indicators in the
header.

## 👩‍💻 Implementation

There were a number of changes that needed to be made within nimble to
facilitate this change.

1. Column Internals
- Add boolean properties of `resizingDisabled` and
`hideHeaderIndicators`
1. Table Update Tracker
- Mark `columnWidths` as being changed when `resizingDisabled` changes
in `columnInternals`
1. Table Layout Manager
    - Update to deal with columns that have `resizingDisabled === true`
1. Table
- Add logic to hide column dividers that can never be dragged to the
left or to the right. This is accomplished by adding a `draggable` class
to the dividers that can be dragged and making the presence of that
class a condition in the CSS for showing the divider.
- Add logic to set `indicators-hidden` on table columns where
`hideHeaderIndicators === true`
1. Table Header
- Add a boolean attribute of `indicators-hidden` that, when `true`,
hides the sorting & grouping indicators in the column header.
1. Icon Column
- Add `width-mode` attribute. When the attribute is changed,
`columnInternals` is updated with appropriate values for
`resizingDisabled`, `hideHeaderIndicators`, `minPixelWidth`, and
`pixelWidth`.

## 🧪 Testing

Manually tested in storybook
Updated & added unit 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: Jonathan Meyer <26874831+atmgrifter00@users.noreply.github.com>
Co-authored-by: Milan Raj <rajsite@users.noreply.github.com>
@mollykreis mollykreis mentioned this pull request May 7, 2024
Merged
1 task
mollykreis added a commit that referenced this pull request May 10, 2024
@mollykreis
Create nimble-mapping-empty component (#2077)
090b724 
# Pull Request

## 🤨 Rationale

As part of #1992, we decided to create a `nimble-mapping-empty`
component that would allow a cell to render as blank for a given mapping
while still having text provided for use in group rows.

This PR creates that component within `nimble-components`. Support for
Blazor and Angular will be added in a subsequent PR.

## 👩‍💻 Implementation

- Create `nimble-mapping-empty` component
- Add support for the component in the mapping column
- Update tests

## 🧪 Testing

- Extended the unit tests for the mapping column to include tests for
empty mappings
- Add empty mapping to the matrix test for the mapping column

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

@jattasNI jattasNI jattasNI approved these changes

@rajsite rajsite rajsite approved these changes

@atmgrifter00 atmgrifter00 atmgrifter00 approved these changes

@m-akinc m-akinc m-akinc approved these changes

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 @rajsite @m-akinc @jattasNI @atmgrifter00