-
Notifications
You must be signed in to change notification settings - Fork 192
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
Conversation
Lighthouse scores
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 Transfer Size
Request Count
|
Tachometer resultsChromecolor-field permalinkbasic-test
combobox permalinkbasic-test
light-dom-test permalink
Firefoxcolor-field permalinkbasic-test
combobox permalinkbasic-test
light-dom-test permalink
|
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 I'm looking for comments on the |
There was a problem hiding this 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.
<section> | ||
${this.headline && | ||
html` | ||
<h2 class="heading">${this.headline}</h2> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😓
There was a problem hiding this comment.
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....
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than a dev mode warning I would like to get it surfaced up to the documentation site to have more visibility
></sp-icon-info-outline> | ||
`} | ||
</sp-action-button> | ||
<sp-overlay |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 aTriggerController
that held theopen
state for you to be able to render with (feels either really smart of trying too hard) - other (better) things...
There was a problem hiding this comment.
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 :(
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@Rajdeepc as per office hours discussion, I removed the |
@TarunAdobe seems to be due to the type of the controls: |
@Rocss Hmm seems like you're right! Thanks for pointing it out. We will take this up in a separate PR as this needs some work around how we create these demos. |
`} | ||
</sp-action-button> | ||
<sp-overlay | ||
trigger="trigger@click" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Rocss Updates on this? Do you think using DependencyManagerController
here would benefit the component?
await nextFrame(); | ||
await nextFrame(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to wait for a few frames here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Waiting for DOM clean up to complete, similar to here. The test fails without it because it still finds the popover in the DOM.
Looks like vrts's are failing! |
@blunteshwar I observe this, however, the failing tests are not related to this PR, and the error seems to be for Action Button |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!! We will work on adding a dependency
manager (if feasible and required) in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for bringing this in!
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?
info
/help
variantsplacement
prop using the Storybook Controls Addonplacement
propopen
controlScreenshots (if appropriate)
Types of changes
Checklist
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
.