Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
base: main
Are you sure you want to change the base?
feat(contextual-help): add contextual help pattern #4285
Changes from 4 commits
ce0e828
5d8b600
d5cd252
fde83c0
9d49c2e
46da0af
cba493a
1af04eb
d192b57
9082919
2dcce90
5fc11bf
9564c00
e3f26a4
26c12c4
3ba3ad3
c488283
598c1f9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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:
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
ordiv
. 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
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 addactive
attribute on the trigger as a built-in feature of thetrigger
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 toggleopen
which is rendered toactive
. In API extensions, I'd love to chat more on the possibilities around:handleOpen
/handleClose
callbacks to the options bagtrigger
directive as aTriggerController
that held theopen
state for you to be able to render with (feels either really smart of trying too hard)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
andhandleClose
callback, asslottable-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 withhandleOpen
/handleClose
would be if the timing of calling these callbacks should be on thehandleSlottableRequest
method, or a bit late, onsp-open
andsp-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
andsp-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.
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.
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.
Moving to
open
likely means moving to.triggerElement=...
instead and?open=${this.open}
which them means you might want to leverageDependencyManagerController
as seen inPicker
.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?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.
@tarun I don't understand why moving to
open
would mean moving to.triggerElement=...
instead.I tried using the
DependencyManagerController
but I found out it does not play well with theslottable-request
event:You need to open the
sp-overlay
in order to register the dependencies, but you can not open it unless the dependencies are registered. (?open=${this.open &&this.dependencyManager.loaded}
)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.