Fix tag-list-item clearable functionality #3772
Conversation
|
Thanks for the PR! 🎉 We've deployed an automatic preview for this PR - you can see your changes here:
|
|
|
||
| /** Dispatched when a user selects to delete an individual tag list item. The consumer must handle the actual element deletion and focus behaviour if there are no remaining list items. */ | ||
| this.dispatchEvent(new CustomEvent( | ||
| 'd2l-tag-list-item-clear', | ||
| { bubbles: true, composed: true, detail: { value: this.text } } | ||
| { bubbles: true, composed: true, detail: { key: this.key } } |
There was a problem hiding this comment.
I don't think there's much value in this.
| { bubbles: true, composed: true, detail: { key: this.key } } | |
| { bubbles: true, composed: true } |
There was a problem hiding this comment.
Yeah I agree, but do you think it'd help people "do the right thing"? Encourages using the event detail to get the data from the data array, rather than hooking up an anonymous function to each item passing in data like a few places are doing?
There was a problem hiding this comment.
Yeah, I guess it could, and it doesn't hurt any to leave it here.
There was a problem hiding this comment.
I'm not sure it's going to prevent that. If they're not inclined to use e.target.key then I suspect they won't use e.detail.key either. I usually opt for not duplicating the data that just as easily accessed, but as you said, it doesn't really hurt.
There was a problem hiding this comment.
To close the loop on this - per conversation offline, it looks like there are a number of other components that do provide the key on the event detail as a matter of convenience. I think we decided to follow suit, yeah?
| @@ -71,8 +71,8 @@ <h2>Clearable Tag List</h2> | |||
| import { getNextFocusable } from '../../../helpers/focus.js'; | |||
|
|
|||
| document.addEventListener('d2l-tag-list-item-clear', (e) => { | |||
| console.log(`d2l-tag-list-item-clear event dispatched. Value: ${e.target.text}`); | |||
There was a problem hiding this comment.
Should we add keys to the demo, as a good example? And then we can add it here alongside the value, to show how to get both
| * Acts as a unique identifier for the tag | ||
| * @type {string} | ||
| */ | ||
| key: { type: String }, |
There was a problem hiding this comment.
Do we want to mark this as "Required" in the docs? 🤔 It doesn't really break anything if you don't have it, but the event does expect it.
| @@ -91,6 +91,24 @@ describe('d2l-tag-list-item', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| describe('display text', () => { | |||
| it('should default to tagContent', async() => { | |||
There was a problem hiding this comment.
| it('should default to tagContent', async() => { | |
| it('should use text for tag-list-item', async() => { |
Looks like this isn't really accurate anymore, maybe something like this?
bearfriend
force-pushed
the
dgleckler/tag-list-item-clear
branch
2 times, most recently
from
d83c7d4
to
6a4f913
Compare
|
Could not generate new goldens - your code changes will update golden files that you do not have the latest version of. Please rebase or merge |
bearfriend
force-pushed
the
dgleckler/tag-list-item-clear
branch
from
7cda3e5
to
1cbbea4
Compare
|
Visual diff tests failed - pull request #3789 has been opened with the updated goldens. |
Co-authored-by: github-actions <[email protected]>
| _renderTag(tagContent, options) { | ||
| if (!options) options = {}; | ||
| _renderTag(tagContent, options = {}) { | ||
| if (options.labelText?.constructor !== String) throw new TypeError('options.labelText must be a string'); |
There was a problem hiding this comment.
Oh, I guess we'll want to have the users PR ready to go to merge right after this? Or the user tag items will start throwing?
|
🎉 This PR is included in version 2.131.7 🎉 The release is available on: Your semantic-release bot 📦🚀 |
ghost
added
the
DE53055: Fixer > User Tag List Items > Fix clearable functionality
Many consumers of
d2l-tag-list-itemandTagListItemMixinuse custom (usually data) attributes on tags to identify them. This PR adds akeyproperty to thetag-list-item-mixinto act as a unique identifier for each tag, and includes it in thed2l-tag-list-item-clearevent (though I'm not sure how valuable it really is there sincee.target.keyis already available).Sometimes there are additional attributes added to denote relationships to other objects/entities. This seems like a reasonable and appropriate practice to me that does not require facilitation through the component/mixin.
Additionally, and probably more importantly, I've added a
displayTextoption to_renderTagwhich allows consumers to give the tag a simple label for use within various langterms and screenreader announcements. This is useful when the contents of the tag are not a simple string.The only place that uses
_renderTagwith non-string contents already doesn't announce or localize properly, so this should be updated to passdisplayText, but it can be done at any time:BrightspaceHypermediaComponents/users
Changing the event
detailis technically a breaking change, but nothing is actually using the existingdetail.value, so these uses ofd2l-tag-list-item-clearcan be updated to usekeybut are not required:Brightspace/insights-adoption-dashboard
Brightspace/d2l-outcomes
Brightspace/cds-reflection-tool
🤷Weird things I kinda wish I didn't see:Brightspace/nova
Brightspace/nova
Brightspace/cds-custom-pacing-tool-ui