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

feat(contextual-help): add contextual help pattern #4285

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

Conversation

Rocss
Copy link
Contributor

@Rocss Rocss commented Apr 22, 2024

Description

Adds @spectrum-web-components/contextual-help, as per https://spectrum.adobe.com/page/contextual-help/

Related issue(s)

Motivation and context

Contextual help is a pattern used widely across Adobe apps. Even if at its core it is a simple component, the lack of a reusable component around this pattern results in different teams implementing different versions of it.
Differences in implementation differ not only on the visual level, but also on the accessibility and mobile responsiveness level. Current implementations are some of them lacking keyboard navigation support, screen reader support, and not taking into consideration user experience on mobile devices.

How has this been tested?

  • Test case 1
    1. Go here
    2. Do this
  • Test case 2
    1. Go here
    2. Do this

Screenshots (if appropriate)

Screenshot 2024-04-22 at 11 28 57

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Checklist

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • If my change required a change to the documentation, I have updated the documentation in this pull request.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have reviewed at the Accessibility Practices for this feature, see: Aria Practices

Best practices

This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against main.

Copy link

github-actions bot commented Apr 22, 2024

Lighthouse scores

Category Latest (report) Main (report) Branch (report)
Performance 0.99 0.94 0.97
Accessibility 1 1 1
Best Practices 1 1 1
SEO 1 0.92 0.92
PWA 1 1 1
What is this?

Lighthouse scores comparing the documentation site built from the PR ("Branch") to that of the production documentation site ("Latest") and the build currently on main ("Main"). Higher scores are better, but note that the SEO scores on Netlify URLs are artifically constrained to 0.92.

Transfer Size

Category Latest Main Branch
Total 221.084 kB 210.603 kB 🏆 218.269 kB
Scripts 53.285 kB 48.425 kB 🏆 55.396 kB
Stylesheet 34.913 kB 30.374 kB 🏆 30.861 kB
Document 5.92 kB 5.189 kB 🏆 5.30 kB
Font 126.966 kB 126.615 kB 🏆 126.712 kB

Request Count

Category Latest Main Branch
Total 45 45 44 🏆
Scripts 37 37 36 🏆
Stylesheet 5 5 5
Document 1 1 1
Font 2 2 2

Copy link

github-actions bot commented Apr 22, 2024

Tachometer results

Chrome

action-bar permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 484 kB 48.64ms - 51.19ms - faster ✔
1% - 8%
0.55ms - 4.20ms
branch 472 kB 50.98ms - 53.60ms slower ❌
1% - 9%
0.55ms - 4.20ms
-

action-menu permalink

test-basic

Version Bytes Avg Time vs remote vs branch
npm latest 646 kB 129.11ms - 132.52ms - faster ✔
4% - 8%
6.15ms - 10.94ms
branch 634 kB 137.68ms - 141.04ms slower ❌
5% - 8%
6.15ms - 10.94ms
-

test-directive permalink

Version Bytes Avg Time vs remote vs branch
npm latest 603 kB 59.49ms - 60.60ms - faster ✔
6% - 9%
3.85ms - 5.83ms
branch 592 kB 64.06ms - 65.70ms slower ❌
6% - 10%
3.85ms - 5.83ms
-

test-lazy permalink

Version Bytes Avg Time vs remote vs branch
npm latest 602 kB 58.79ms - 60.13ms - faster ✔
4% - 7%
2.65ms - 4.77ms
branch 591 kB 62.35ms - 63.99ms slower ❌
4% - 8%
2.65ms - 4.77ms
-

test-open-close-directive permalink

Version Bytes Avg Time vs remote vs branch
npm latest 788 kB 1872.68ms - 1875.70ms - unsure 🔍
-0% - +0%
-2.00ms - +2.32ms
branch 777 kB 1872.48ms - 1875.57ms unsure 🔍
-0% - +0%
-2.32ms - +2.00ms
-

test-open-close permalink

Version Bytes Avg Time vs remote vs branch
npm latest 787 kB 1857.33ms - 1859.89ms - unsure 🔍
-0% - +0%
-2.45ms - +1.01ms
branch 775 kB 1858.17ms - 1860.49ms unsure 🔍
-0% - +0%
-1.01ms - +2.45ms
-

combobox permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 709 kB 35.44ms - 35.81ms - faster ✔
3% - 4%
1.02ms - 1.58ms
branch 697 kB 36.71ms - 37.13ms slower ❌
3% - 4%
1.02ms - 1.58ms
-

light-dom-test permalink

Version Bytes Avg Time vs remote vs branch
npm latest 709 kB 370.42ms - 376.18ms - faster ✔
3% - 5%
12.29ms - 20.67ms
branch 697 kB 386.74ms - 392.83ms slower ❌
3% - 6%
12.29ms - 20.67ms
-
Firefox

action-bar permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 484 kB 111.80ms - 118.00ms - faster ✔
1% - 7%
0.57ms - 8.67ms
branch 472 kB 116.90ms - 122.14ms slower ❌
0% - 8%
0.57ms - 8.67ms
-

action-menu permalink

test-basic

Version Bytes Avg Time vs remote vs branch
npm latest 646 kB 279.20ms - 282.28ms - faster ✔
13% - 14%
41.29ms - 47.19ms
branch 634 kB 322.47ms - 327.49ms slower ❌
15% - 17%
41.29ms - 47.19ms
-

test-directive permalink

Version Bytes Avg Time vs remote vs branch
npm latest 603 kB 133.12ms - 136.40ms - unsure 🔍
-1% - +1%
-1.88ms - +1.72ms
branch 592 kB 134.09ms - 135.59ms unsure 🔍
-1% - +1%
-1.72ms - +1.88ms
-

test-lazy permalink

Version Bytes Avg Time vs remote vs branch
npm latest 602 kB 154.58ms - 161.94ms - slower ❌
10% - 16%
13.77ms - 22.23ms
branch 591 kB 138.17ms - 142.35ms faster ✔
9% - 14%
13.77ms - 22.23ms
-

test-open-close-directive permalink

Version Bytes Avg Time vs remote vs branch
npm latest 788 kB 1901.09ms - 1911.55ms - slower ❌
1% - 1%
11.69ms - 22.99ms
branch 777 kB 1886.85ms - 1891.11ms faster ✔
1% - 1%
11.69ms - 22.99ms
-

test-open-close permalink

Version Bytes Avg Time vs remote vs branch
npm latest 787 kB 1884.99ms - 1889.93ms - unsure 🔍
-0% - +0%
-5.22ms - +3.30ms
branch 775 kB 1884.95ms - 1891.89ms unsure 🔍
-0% - +0%
-3.30ms - +5.22ms
-

combobox permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 709 kB 59.95ms - 62.57ms - unsure 🔍
-4% - +1%
-2.18ms - +0.62ms
branch 697 kB 61.54ms - 62.54ms unsure 🔍
-1% - +4%
-0.62ms - +2.18ms
-

light-dom-test permalink

Version Bytes Avg Time vs remote vs branch
npm latest 709 kB 745.64ms - 761.52ms - slower ❌
1% - 5%
6.71ms - 34.17ms
branch 697 kB 721.94ms - 744.34ms faster ✔
1% - 5%
6.71ms - 34.17ms
-

@Rocss Rocss changed the title feat(contextual-help): add ContextualHelp component feat(contextual-help): add contextual help pattern Apr 22, 2024
@Rocss Rocss marked this pull request as ready for review April 22, 2024 09:27
@Rocss
Copy link
Contributor Author

Rocss commented Apr 22, 2024

I'm marking it as ready for review even if unit tests are incomplete, because I am looking for a high level review of this current implementation, and any changes would make the tests obsolete.

I found this component to be a good use case for the slottable-request pattern. However, if this seems overkill given the small contents of the popover I can get rid of it.

I'm looking for comments on the width / max-width dilema: #4281 (comment), as well as on the mobile behaviour.

@Rocss Rocss requested a review from Rajdeepc April 22, 2024 09:32
Copy link
Collaborator

@Westbrook Westbrook left a comment

Choose a reason for hiding this comment

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

You can confirm the args and template sharing by seeing the demo published at the top of the docs site after the change.

packages/contextual-help/src/ContextualHelp.ts Outdated Show resolved Hide resolved
<section>
${this.headline &&
html`
<h2 class="heading">${this.headline}</h2>
Copy link
Collaborator

Choose a reason for hiding this comment

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

A user should have control over the heading level leveraged here. They should also have the ability to render HTML into this content.

Likely we want something like:

<div class="heading"> <!-- holds style delivery -->
  <slot name="heading"> <!-- normalize this name to that in other elements -->
    <h2>${this.heading}</h2> <!-- holds default headline level but reverts default heading styles -->
  </slot>
</div>

Or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually did not use a slot because of the fact that users have too much flexibility on the heading here and can actually use other things besides heading elements, such as p or div. I wanted to kind of enforce this one to be a heading. I can of course replace it with a slot.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a really difficult balance to tread. We've been asked in the past by the accessibility team to ensure that patterns like this can accept multiple levels of heading to ensure the content is accessible, even if we haven't always gotten back to the related fixes 🙈. If there were other practical approaches to ensuring default delivery AND supporting varying levels of headlines, any new insight into the conversation that you might have would be greatly appreciated.

strictness vs support vs flexibility 😓

Copy link
Contributor Author

@Rocss Rocss Apr 23, 2024

Choose a reason for hiding this comment

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

Added a heading slot in the end.
Can we throw a warning if the slotted element is not a heading element (not h1,2,3... or role="heading")?

LE: well actually you can add nested slotted element which has a heading inside....

Copy link
Collaborator

Choose a reason for hiding this comment

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

I kind of like that as an option, though the work to do so is non-trivial. Might be worth creating an issue to come back to that at a later date?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll create an issue for this. Do you see this being useful for other components as well, such as Dialogs?

packages/contextual-help/src/ContextualHelp.ts Outdated Show resolved Hide resolved
packages/contextual-help/src/ContextualHelp.ts Outdated Show resolved Hide resolved
packages/contextual-help/src/ContextualHelp.ts Outdated Show resolved Hide resolved
packages/contextual-help/src/ContextualHelp.ts Outdated Show resolved Hide resolved
packages/contextual-help/src/contextual-help.css Outdated Show resolved Hide resolved
--spectrum-contextual-help-body-color: var(--spectrum-body-color);
}

.spectrum--large .popover {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems like it doesn't belong... It should be processed out on it's own, but it may exist outside of our current CSS processing. Can you confirm the selector in the origin CSS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here it is: https://github.com/adobe/spectrum-css/blob/main/components/contextualhelp/index.css#L25 I can not find other components with this spectrum--large class. Maybe the --spectrum-contextual-help-content-spacing variable should be moved in the large-vars and medium-vars?

></sp-icon-info-outline>
`}
</sp-action-button>
<sp-overlay
Copy link
Collaborator

Choose a reason for hiding this comment

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

We may also want to look at the trigger directive here, as it lists as slightly more performant that leveraging the element.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not aware of this directive. I think it is really cool that is handles the slottable-request event for you.
A thing I did not found straight-forward is the open/close events. Is there a better way to know when it opens/closes events besides adding event listeners on the ContextualHelp class?
I'd need to know of open/close states for the button's active state. Would you consider a good addition to add active attribute on the trigger as a built-in feature of the trigger directive?

Copy link
Collaborator

@Westbrook Westbrook Apr 23, 2024

Choose a reason for hiding this comment

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

It's not even "brand new" quite yet, as we're working through its specifics internally and with some select early clients before we mark it as "for consumption". Some approach to this seems like an important part of this API, but I'm not sure direct manual handling of specific attributes inside of the directive is the right thing to do here.

While not pretty, that best I can think of with the established API is to use specific handing of slottable-request to toggle open which is rendered to active. In API extensions, I'd love to chat more on the possibilities around:

  • adding handleOpen/handleClose callbacks to the options bag
  • advanced used of the trigger directive as a TriggerController that held the open state for you to be able to render with (feels either really smart of trying too hard)
  • other (better) things...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As a consumer, the first thing I looked for was a handleOpen and handleClose callback, as slottable-request event is part of the internal works of this directive and I don't know if as a consumer I should be aware of that.

sp-open/sp-close events are fine, but are actually a bit delayed, because of the CSS transitions that have dispatched I guess. So the question with handleOpen / handleClose would be if the timing of calling these callbacks should be on the handleSlottableRequest method, or a bit late, on sp-open and sp-closed.

In my case as you mentioned sp-open/sp-close are a bit late :(

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, sp-open and sp-close are not "correct for this situation.

If we went with callbacks, they would likely happen at slottable-request time, because that represents the outside of the open/close transition, rather than only the end of each transition. But, it could be argued that this belongs at the beginning of each transition, which is when the state actually changes. 🤔

Relying on the state feels nicer than relying on a knowable part of the lifecycle, which is why I like the TriggerController, but there is much to think about here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that best I can think of with the established API is to use specific handing of slottable-request to toggle open which is rendered to active.

Regarding this, I don't think this works, as the event is attached on the sp-overlay, which I don't have access to.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed. So we can ship once and edit later leveraging <sp-overlay> directly, or we can hold this for an expansion of the directives capabilities. Do you have a preference on that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Depending on when exactly would the directive be expanded. If it's more than 1-2 releases, then I'd ship it as it is and come back when this behaviour is available.

@sp-closed=${() => {
this.overlayOpen = false;
}}
@slottable-request=${this.isMobile.matches
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you want to also offer the option for a consumer to lazily render the content of this overlay?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This does not seem like the kind of component you interact with way too many times, as its content does not change often.
So I am thinking it would be a good practice to lazy load its content. On the other hand, its content is indeed modest. What is your opinion on this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we'd want it at some point. It is a performance optimization, so I'm OK with excluding it for the initial release, but prefer to take your lead as you're the one actually doing the work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd ship it as it is for a component which theoretically should have just some text inside.

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.

None yet

2 participants