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

Add API investigation details to tooltip spec #1089

Closed
wants to merge 5 commits into from

Conversation

m-akinc
Copy link
Contributor

@m-akinc m-akinc commented Feb 28, 2023

Pull Request

🀨 Rationale

There are some open design decisions that need to be addressed before the tooltip is ready for general use. This spec update attempts to resolve those issues.

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

See spec.

πŸ§ͺ Testing

N/A

βœ… Checklist

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

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.

Spec does not have any discussion of alternatives. May want to have a meeting to brainstorm api options if have not had previous discussions.

@m-akinc m-akinc changed the title Update tooltip spec Add API investigation details to tooltip spec Mar 1, 2023
@m-akinc m-akinc requested a review from rajsite March 1, 2023 20:59

Below we enumerate several different design approaches from which we may choose one or more as our ideal API surface.

### Prior art
Copy link
Contributor

Choose a reason for hiding this comment

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

Prior art seems like the wrong section title here. Maybe something like Existing APIs or Example APIs or even very explicitly APIs from other design systems.

- Familiar pattern, e.g. `<label>` element with `for` attribute, `aria-labelledby` attribute, `aria-describedby` attribute
- Reasonable DOM structure, i.e. tooltip sibling of anchor element

**Cons**
Copy link
Contributor

Choose a reason for hiding this comment

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

The other con we were going to list here was that there we didn't get any accessibility benefits for free with this approach (i.e. we shouldn't automatically make the tooltip describe the anchor element).


**Cons**
- Expensive to implement and maintain. <span style="color:red">**Requires forking and modifying the template of every component that wants to support a tooltip.**</span> (Though code duplication could be avoided by using shared template fragments.) Cannot leverage FAST implementation?
- <span style="color:red">**Does not support rich tooltip content (which is probably a requirement)**</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be worth noting here that it isn't just "rich tooltip content" that isn't supported -- we also lose support for tooltip icons & severity. Or, to keep support, you'd have to have a tooltip attribute plus additional attributes for configuration of the tooltip (e.g. tooltip-severity and tooltip-icon-visible).

- May not need `nimble-tooltip` element at all -- could slot tooltip content directly without wrapping in `nimble-tooltip` element

**Cons**
- Expensive to implement and maintain. <span style="color:red">**Requires forking and modifying the template of every component that wants to support a tooltip.**</span> (Though code duplication could be avoided by using shared template fragments.) Tooltip implementation leaks into every other component.
Copy link
Member

Choose a reason for hiding this comment

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

As we discussed we have added slots to elements without forking templates, for examples the text field:

end: html<TextField>`
<${DesignSystem.tagFor(IconExclamationMark)}
severity="error"
class="error-icon"
></${DesignSystem.tagFor(IconExclamationMark)}>
<span part="actions">
<slot name="actions"></slot>
</span>
${errorTextTemplate}
`

So I'm not sold on the assertion that every template must be forked for us to add new slots to our components, even today.

Tooltip implementation leaks into every other component.

I'm not exactly sure what are the cons this statement is trying to express. We call having a start slot that supports an icon a feature not a leak of icon implementation into components. Is there some concern this is expressing?

Suggested change
- Expensive to implement and maintain. <span style="color:red">**Requires forking and modifying the template of every component that wants to support a tooltip.**</span> (Though code duplication could be avoided by using shared template fragments.) Tooltip implementation leaks into every other component.
- Requires per element modification to add new tooltip slots. While we have added new slots to existing elements, there may be situations where it is required to fork a template to support adding a tooltip slot. The exact cost would need prototyping / investigation.

Copy link
Member

Choose a reason for hiding this comment

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

Based on the other discussion I'm actually a bit more convinced that while still per-component work, we could implement this pattern today with just a slightly less optimal pattern by dynamically inserting slots into templates.

- No dedicated tooltip component

**Cons**
- Expensive to implement and maintain. <span style="color:red">**Requires forking and modifying the template of every component that wants to support a tooltip.**</span> (Though code duplication could be avoided by using shared template fragments.) Cannot leverage FAST implementation?
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is true. We can easily add new attributes to controls and make shared patterns for adding attributes. It's also very possible that we don't necessitate a template change in tooltip logic. The tooltip logic could be very standalone, ie register listeners on elements in the template and show the tooltip dynamically (document.createElement the tooltip and insert it).


**Cons**
- Annoying to have to give anchor element an `id`
- <span style="color:red">**Not conducive to use in shared components because `id` values must be unique**</span>
Copy link
Member

Choose a reason for hiding this comment

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

I'll mention it in one spot but please remove the html style formatting with <span style="color:red">. It doesn't render on GitHub and makes the markdown harder to read.


**Pros**
- Easy to implement
- Proven pattern: used by Carbon and Atlassian Design Systems
Copy link
Member

Choose a reason for hiding this comment

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

I think "proven" is a strong assertion, I'd be interested in knowing if those design system authors and their users actually prefer that pattern and if so how they came to that design.

Suggested change
- Proven pattern: used by Carbon and Atlassian Design Systems
- Pattern used by Carbon and Atlassian Design Systems


**Pros**
- Easy to implement
- Proven pattern: used by Adobe Spectrum Design System
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Proven pattern: used by Adobe Spectrum Design System
- Pattern used by Adobe Spectrum Design System

**Pros**
- Easy to implement
- Proven pattern: used by Adobe Spectrum Design System
- Reasonable DOM structure, i.e. tooltip sibling of anchor element
Copy link
Member

Choose a reason for hiding this comment

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

It's only "reasonable" compared to the previous example

Suggested change
- Reasonable DOM structure, i.e. tooltip sibling of anchor element
- Elements as siblings is less counter-intuitive than the "Tooltip takes anchor element as content" example.

@@ -38,7 +134,12 @@ The tooltip will have a custom template based on FAST's template. In addition to

### Future Improvements

Copy link
Member

Choose a reason for hiding this comment

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

I think we need an interactions / UX discussion on mobile and touch compatibility of tooltips as well as keyboard accessibility of tooltips.

@rajsite
Copy link
Member

rajsite commented Apr 6, 2023

@m-akinc if this is not being actively worked on can you switch to draft?

@m-akinc m-akinc marked this pull request as draft April 6, 2023 16:40
@rajsite
Copy link
Member

rajsite commented May 1, 2023

@m-akinc this issue has not been active for about a month, closing, can be re-opened to address concerns

@rajsite rajsite closed this May 1, 2023
@m-akinc m-akinc deleted the users/makinc/tooltip-spec branch December 1, 2023 22:26
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