Mapping anchor column HLD #2533
Conversation
atmgrifter00
requested review from
rajsite,
jattasNI and
mollykreis
as code owners
| - 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 |
There was a problem hiding this comment.
Lol, I think that is the exact goal of this PR, to specify text outside the mapping :P
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oops...that was just a copy-pasta goof.
|
|
||
| --- | ||
|
|
||
| ## Open Issues |
There was a problem hiding this comment.
Do we think we will have a visual design pass? My biggest question was how an icon + icon text + link should look together @fredvisser
There was a problem hiding this comment.
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).
- 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:
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. - 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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Example of current systems table:
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| ##### 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` |
There was a problem hiding this comment.
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 otheranchor-*elements maybe - a mixin for AnchorColumns which calls the above mixin and adds
label-filed-name,href-field-name,appearance,underline
There was a problem hiding this comment.
Cool. I think I'll leave that out of this HLD. May want to discuss details of this idea out-of-band.
| - Supported input: | ||
| - string | ||
| - number | ||
| - boolean |
There was a problem hiding this comment.
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
| </span> | ||
| ``` | ||
|
|
||
| ### Sorting |
There was a problem hiding this comment.
Can you add a similar section for grouping behavior?
|
|
||
| #### 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. |
There was a problem hiding this comment.
Should this say "Using the TableColumnMappingAnchor"? Or is this saying the TableColumnAnchor will somehow be reused?
There was a problem hiding this comment.
Think the intention was something along the lines of TableColumnMappingAnchor will follow the similar NavigationGuard directive pattern of TableColumnAnchor
|
|
||
| --- | ||
|
|
||
| ## Open Issues |
There was a problem hiding this comment.
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).
|
|
||
| ### API | ||
|
|
||
| #### Enum column element: |
There was a problem hiding this comment.
| #### Enum column element: | |
| #### Enum column element: |
| #### Enum column element: | |
| #### Mapping anchor column element: |
|
|
||
| --- | ||
|
|
||
| ## Open Issues |
There was a problem hiding this comment.
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.
| @@ -0,0 +1,197 @@ | |||
| # Mapping Anchor Table Column | |||
There was a problem hiding this comment.
Once this is closer to ready for approval, we should add Stefan as a required reviewer.
Pull Request
🤨 Rationale