Skip to content

Mapping anchor column HLD #2533

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Mapping anchor column HLD #2533

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3a7bc9b
Initial HLD draft.
atmgrifter00 Feb 5, 2025
e09ceee
Linting.
atmgrifter00 Feb 5, 2025
f3a82bb
Merge branch 'main' into mapping-anchor-column-HLD
rajsite Mar 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 2 additions & 6 deletions ...nimble-components/src/table/specs/table-column-specs/table-column-anchor-hld.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,6 @@ When grouped, the header item should not be a link.

The column will also support having a custom sort order, as detailed in the [Custom Sort Order HLD](../table-column-custom-sort-field-hld.md).

### Clearing state

Because the anchor in the cell can have focus, we must override `focusedRecycleCallback()` in our cell view and have it call `blur()` on our anchor. If we don't, the focus can pass to other cells as you scroll.

### Sizing

The column should support the same sizing modes as the text column, which is fractional widths plus minimum pixel widths.
Expand All @@ -84,9 +80,9 @@ Arrowing to a anchor table cell should focus the link (if it is actually a link

In the accessibility tree, the cells of an anchor column are instances of [`nimble-table-cell`](https://github.com/ni/nimble/blob/f663c38741e731bef91aa58e8fb2d1cec653b679/packages/nimble-components/src/table/components/cell/template.ts#L6) which has a `role` of [`cell`](https://w3c.github.io/aria/#cell). The cell then has a child `nimble-anchor`, which has a `role` of [`link`](https://w3c.github.io/aria/#link).

### Angular RouterLink Support
### Angular Routing support

The real challenge of this column type is integrating with the Angular router. The `RouterLink`/`RouterLinkWithHref` directives are used to intercept clicks on anchors and replace the default navigation action with a call to `Router.navigateByUrl()`. As we have done in the past for other anchor components, we will have our own directive deriving from `RouterLinkWithHref`. Our directive will apply based on the presence of the `nimbleRouterLink` attribute. As we have done in the past, we will also have a directive that throws an error if `routerLink` is used instead. Normally, you would specify the `nimbleRouterLink` and related attributes (e.g. `queryParams`, `replaceUrl`, etc.) directly on the anchor element, but that's not possible for anchors in generated table cells. Our options are to put the directive on `nimble-table` or `nimble-table-column-anchor`. Because we would like to allow different anchor columns to be configured differently, and because it is a more intuitive API, we want to put our directive on the column element:
Using the `TableColumnAnchor` in Angular requires the use of the [`NavigationGuard`](https://github.com/ni/nimble/blob/main/packages/angular-workspace/nimble-angular/table-column/anchor/nimble-table-column-anchor-navigation-guard.directive.ts) in order to allow proper routing within the Angular application.

```html
<nimble-table>
Expand Down
197 changes: 197 additions & 0 deletions ...le-components/src/table/specs/table-column-specs/table-column-mapping-anchor.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,197 @@
# Mapping Anchor Table Column
Copy link
Contributor

Choose a reason for hiding this comment

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

Once this is closer to ready for approval, we should add Stefan as a required reviewer.


## Overview

The `nimble-table-column-mapping-anchor` is a component that supports rendering specific number, boolean, or string values as an icon or a spinner, with an accompanying text anchor element. The mappings are defined by child elements of `nimble-mapping-icon`, `nimble-mapping-spinner`, or `nimble-mapping-empty`.

### Background

[Icon with anchor column](https://github.com/ni/nimble/issues/2531)

### Features

- Supported input:
- string
- number
- boolean
Comment on lines +13 to +16
Copy link
Contributor

Choose a reason for hiding this comment

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

This statement is a bit confusing (at least to me). Since there are multiple inputs to the column that support different types, it might be more helpful to split out the "Supported input" section into what is supported for a given input. Something like:

  • Supported input:
    • string, number, or boolean mapping
    • string label
    • string href

- Supported output:
- Icon with text link
- Spinner with text link
- text link

### Non-goals

- Non-Nimble icon support
- Arbitrary icon colors
- Non-icon, non-spinner Nimble components
- Specifying different text for an icon/spinner's label than the overall text of the mapping
Copy link
Member

Choose a reason for hiding this comment

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

Lol, I think that is the exact goal of this PR, to specify text outside the mapping :P

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe Milan's right, that's the main reason we created that issue. The current requirement is to have both the text next to the icon, as well as the text displayed in the tooltip when hovered over display the same text. If a different decision is taken for this feature, I would like to get the confirmation of the PO first that this is acceptable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops...that was just a copy-pasta goof.


---

## Design

Below is an example of how these elements would be used within a `nimble-table`:

```HTML
<nimble-table>
<nimble-table-column-mapping-anchor field-name="status" key-type="string" label-field-name="status" href-field-name="status-url">
Status
<nimble-mapping-icon key="fail" icon="nimble-icon-xmark" severity="error" text="Failed"></nimble-mapping-icon>
<nimble-mapping-icon key="error" icon="nimble-icon-xmark" severity="error" text="Errored"></nimble-mapping-icon>
<nimble-mapping-icon key="pass" icon="nimble-icon-check" severity="success" text="Passed"></nimble-mapping-icon>
<nimble-mapping-spinner key="running" text="Running"></nimble-mapping-spinner>
</nimble-table-column-mapping-anchor>
</nimble-table>
```

The notable distinction between this column type and the `nimble-table-column-mapping` is the `label-field-name` and `href-field-name` attributes, and the absence of supporting the `nimble-mapping-text` as one of its mapping types. The two new attributes are aligned with the `nimble-table-column-anchor` API, and provide the fields in the table record for the text and URL for the displayed link. The `nimble-mapping-text` mapping will not be supported as it seems unnecessary for the use-cases needed.

See the [Mapping Table Column spec](./table-column-mapping.md#design), and the [Anchor Table Column spec](./table-column-anchor-hld.md#implementation--design) for more details related to the design.

### API

#### Enum column element:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#### Enum column element:
#### Enum column element:
Suggested change
#### Enum column element:
#### Mapping anchor column element:


_Component Name_

- `nimble-table-column-mapping-anchor`

_Props/Attrs_

##### Attributes inherited from TableColumnEnumBase

- `field-name`: string
- `key-type`: 'string' | 'number' | 'boolean'

##### Attributes "copied" from TableColumnAnchor (intend to move to mixin)

- `label-field-name`: string
- `href-field-name`: string
- `appearance`: string
- `underline-hidden`: string
- `hreflang`
- `ping`
- `referrerpolicy`
- `rel`
- `target`
- `type`
- `download`
Comment on lines +66 to +78
Copy link
Member

@rajsite rajsite Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Might too much detail for HLD but was thinking:

  • a mixin for the native anchor attributes, i.e. hreflang, ping, target, etc. This could also be re-used in other anchor-* elements maybe
  • a mixin for AnchorColumns which calls the above mixin and adds label-filed-name, href-field-name, appearance, underline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. I think I'll leave that out of this HLD. May want to discuss details of this idea out-of-band.


Attributes from [`FractionalWidth`](https://github.com/ni/nimble/blob/main/packages/nimble-components/src/table-column/mixins/fractional-width-column.ts), [`GroupableColumn`](https://github.com/ni/nimble/blob/main/packages/nimble-components/src/table-column/mixins/groupable-column.ts), and [`Placeholder`](https://github.com/ni/nimble/blob/main/packages/nimble-components/src/table-column/mixins/placeholder.ts) mixins.

_Content_

- column title (icon and/or text)
- 1 or more `nimble-mapping-icon`, `nimble-mapping-spinner`, or `nimble-mapping-empty` elements

### Cell Template

The cell template will essentially be a combination of the `TableColumnMappingCellView` and `TableColumnAnchorCellView`.

```html
<span class="reserve-icon-size"> ${x => x.visualizationTemplate} </span>
When cellRecord.href present
<nimble-anchor
href="${x => x.cellRecord.href}"
hreflang="${x => x.columnConfig.hreflang}"
ping="${x => x.columnConfig.ping}"
referrerpolicy="${x => x.columnConfig.referrerpolicy}"
rel="${x => x.columnConfig.rel}"
target="${x => x.columnConfig.target}"
type="${x => x.columnConfig.type}"
download="${x => x.columnConfig.download}"
underline-hidden
@mouseover="${(x, c) => setTitleWhenOverflow(...)}"
@mouseout="${(x, c) => removeTitle(...)}"
>
${cellState.cellRecord.label ?? cellState.cellRecord.href}
</nimble-anchor>
When cellRecord.href is missing
<span
@mouseover="${(x, c) => setTitleWhenOverflow(...)}"
@mouseout="${(_x, c) => removeTitle(...)}"
>
${cellState.cellRecord.label}
</span>
```

### Sorting
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a similar section for grouping behavior?


Sorting will be based on the record value, following what is described in the [Mapping Table Column spec](./table-column-mapping.md#sorting), meaning the column will ultimately be sorted according to the icon "value" and not by the label or URL (as it is with the anchor column).

### Sizing

The column should support the same sizing modes as the text column, which is fractional widths plus minimum pixel widths.

### Keyboard Interactions

Arrowing to a anchor table cell should focus the link (if it is actually a link and not just a text span). Pressing `Enter` on a focused link will trigger navigation.

### Accessibility Roles

In the accessibility tree, the cells of an anchor column are instances of [`nimble-table-cell`](https://github.com/ni/nimble/blob/f663c38741e731bef91aa58e8fb2d1cec653b679/packages/nimble-components/src/table/components/cell/template.ts#L6) which has a `role` of [`cell`](https://w3c.github.io/aria/#cell). The cell then has a child `nimble-anchor`, which has a `role` of [`link`](https://w3c.github.io/aria/#link).

### Angular integration

Angular directives will be created for the column component. No component has form association, so a `ControlValueAccessor` will not be created.

#### Angular Routing support

Using the `TableColumnAnchor` in Angular requires the use of the [`NavigationGuard`](https://github.com/ni/nimble/blob/main/packages/angular-workspace/nimble-angular/table-column/anchor/nimble-table-column-anchor-navigation-guard.directive.ts) in order to allow proper routing within the Angular application.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this say "Using the TableColumnMappingAnchor"? Or is this saying the TableColumnAnchor will somehow be reused?

Copy link
Member

Choose a reason for hiding this comment

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

Think the intention was something along the lines of TableColumnMappingAnchor will follow the similar NavigationGuard directive pattern of TableColumnAnchor


### Blazor integration

Blazor wrappers will be created for the components. Columns will be generic in the type of the key, and will cascade that type parameter to contained mapping elements (see [`CascadingTypeParameter`](https://learn.microsoft.com/en-us/aspnet/core/blazor/components/generic-type-support?view=aspnetcore-7.0#cascaded-generic-type-support)):

```HTML
<NimbleTableColumnMappingAnchor TKey=string Field="Status" LabelField="StatusField" HrefField="StatusURL">
<NimbleMappingIcon
Key="@("success")"
Icon="nimble-icon-check"
Severity="IconSeverity.Success">
</NimbleMappingIcon>
<NimbleMappingSpinner
Key=@("calculating")>
</NimbleMappingSpinner>
</NimbleTableColumnMapping>
```

### Globalization

All text will be provided by the client and is expected to be localized.

### Security

N/A

### Performance

N/A

### Dependencies

None

### Test Plan

- Unit tests will be written verifying the usual component expectations, plus:
- renders mapping matching the cell value (string, number, and boolean)
- nothing rendered when value matches no mappings
- validation error when non-unique mapping keys exist
- validation error when invalid icon name given
- grouping header for icon column includes text
- Verify manually that the column content appears in the accessibility tree and can be read by a screen reader.
- Verify manually that several mapping columns with thousands of elements scrolls performantly.
- Visual Chromatic tests will be created

### Tooling

N/A

### Documentation

Documented in Storybook

---

## Open Issues
Copy link
Member

Choose a reason for hiding this comment

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

Do we think we will have a visual design pass? My biggest question was how an icon + icon text + link should look together @fredvisser

image

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should support both plain text and a hyperlink in this column (which would make the need for a design moot).

  1. It seems arbitrary to support a sequences of exactly "icon, text, link" and "icon, link", but not "icon, link, text" or "icon, link, text, link", or "icon, text, link, text". E.g. you can't do this:
    image
    Granted, it's already a little aribitrary that you can have only one icon, and it must be on the left, but I think if we support mixed plain text and hyperlinked text, the supported use cases feel that much more limited and specific.
  2. It would be nice to support navigating by clicking the icon. I can see reasonable use cases for a hyperlink icon without text. Depending on the specifics, discoverability might be an argument against this, but I don't think it's categorically a bad idea. And when you have (hyperlink) text next to the icon, I think it would be nice to navigate when you click on any of the cell content, not only the text.

Copy link
Member

@rajsite rajsite Feb 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The icon with it's text description is our recommended configuration: https://nimble.ni.dev/storybook/index.html?path=/docs/components-table-column-mapping--docs#choosing-a-mapping-style

It is conceptual mapping + metadata. The mapping is ideally icon + text but can also be just icon. They together describe the state. The metadata is a hyperlink, plain text, or placeholder text.

It would be nice to support navigating by clicking the icon.

I didn't think I agree, the metadata is a distinct thing from the icon, I.e. state of a system itself vs link to error log. I think some concrete examples would help me understand your proposals

Copy link
Member

Choose a reason for hiding this comment

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

Example of current systems table:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

This could be stale information, but I thought one of the goals was to have a compact icon-only representation while still seeing per-row status information in a tooltip.

From the Proposed Solution on #2531:

Have a textFieldName (or a similar property) that would allow providing a custom text for the column.... With this approach, the mapping icons would be fixed (the fixed set of possible icons) and only the tooltip text would be given dynamically by a different property of the row.

I assume in this case the desire would be to have the icon be clickable.

We could push back on that if we think it's not a good direction, but I can see it being useful for the same reasons that width-mode=iconSize is useful: it takes up less space.

If this is something we support, it'd be another visual design question about how to present the mapping text and the per-row label in the same tooltip.

Copy link
Member

@rajsite rajsite Feb 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This could be stale information, but I thought one of the goals was to have a compact icon-only representation while still seeing per-row status information in a tooltip.

That is not the goal I'm aware of or aligned with discussions in offline threads. To reproduce those resolved discussions here:

When merged in a single column hovering over the icon is still proposed to show the icon grouping (more generic value) next to the descriptive text that is a link (which if clipped in the column will show the tooltip on hover). Having arbitrary strings (vs well-grouped strings) on hover of the icon is not the supported direction.

The primary value add of a new nimble-mapping-link column is to combine these 2 columns into one - the behavior is otherwise the same [an icon with a fairly generic title next to a text link].

I do not think a width-mode=icon-size mode for this column would ever be a supported configuration as it mandates hover interactions as the only way to discern some information. A touch device would need a toggle tip to see the meta data and a column fixed to the width of a single icon would not show the toggle tip and hyperlink.

This could change with a rich toggle tip that has already defined patterns around keyboard + touch + mouse interactions for rich content in the toggle tip (anchors), but that does not exist yet / is not on the backlog in any expected timeframe. And I'd argue at least for this use case there is more value in seeing descriptive text in the table at a glance vs requiring an interaction.

Copy link
Member

Choose a reason for hiding this comment

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

Depending on which way we go on visual design, if we end up not wanting to show icon text for the icon mappings I propose we enforce that with validation. Require in mapping validation that all the icon mappings have "text-hidden" true (as opposed to ignoring the attribute on the mapping or creating a new icon mapping that does not have the unused attribute).

Loading