DSD-1708: TagSet update to allow more properties in the data object

[EdwinGuzman]

Fixes JIRA ticket DSD-1708

This PR does the following:

• Allows for more properties in the individual tagset data object.
• The onClick function now returns the entire tag data object in the callback function argument instead of just the tag's label.

How has this been tested?

Locally and through an example.

Accessibility concerns or updates

• N/A

Checklist:

• I have updated the Storybook documentation accordingly.
• I have added relevant accessibility documentation for this pull request.
• All new and existing tests passed.

Front End Review:

• Review the Vercel preview deployment once it is ready.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
nypl-design-system	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 6, 2024 9:47pm

[EdwinGuzman]

As an example, the returned data is the entire data object for the tag that was clicked.

[charmingduchess]

great! i'm surprised the typing on the onClick doesn't complain about the extra properties.

[EdwinGuzman]

I added [key: string]: string; to the prop definition so it's somewhat open for more values (it just won't work if it was field: 2 since that's a number...)

[charmingduchess]

Returning the entire object is definitely an upgrade! The question of the return value for clear filters is less straightforward. With the tagset as it is, the onchange has to expect categorically different data, either the tag you removed, or a message indicating that the tags have been cleared.
I would advocate for onChange to instead pass all of the current tags, whether the user's action is clearing all or dismissing a single filter. For cohesion across the DS, I think it would be a good idea for this component to behave similarly to the checkbox group. In the checkbox group, the onChange is passed all of the selected values (minus or plus the one you just selected). That way, you can always perform the same action with the parameter that's passed in.
This would be a pretty fundamental change in how the TagSet works, so I understand if it's not in scope for this ticket.

[EdwinGuzman]

This update is intended to be a fundamental change because it's going into v3.0 so breaking change are okay, it just need to be documented.

The suggestion makes sense but to be clear, are these examples correct?

• There are five filters [{id:"1"}, {id:"2"}, {id:"3"}, {id:"4"}, {id:"5"}] and "3" was clicked. The onClick function returns [{id:"1"}, {id:"2"}, {id:"4"}, {id:"5"}].
• There are five filters [{id:"1"}, {id:"2"}, {id:"3"}, {id:"4"}, {id:"5"}] and "Clear Filters" was clicked. The onClick function returns [].

This makes sense. It might also help remove the internal useState and useEffect. I can try this. @bigfishdesign13 what do you think?

[charmingduchess]

Ok! In that case, that example is doing what I'm talking about. And when you say return, I assume you mean that's what gets passed into the onChange?

[EdwinGuzman]

Yes, but it's onClick rather than onChange. I'll add a code snippet in the docs as well. It'll be similar to:

const activefilters = [{id:"1"}, {id:"2"}, {id:"3"}, {id:"4"}, {id:"5"}];
const onClick = (activeTags) => {
  // if the tag with id 3 is clicked, then `activeTags` is
  // [{id:"1"}, {id:"2"}, {id:"4"}, {id:"5"}]

   // if the "Clear Filters" button was clicked, then `activeTags` is
  // []

  // now you do whatever you need to do
};

// ...
render (
  <... >

    <TagSet
        isDismissible
        onClick={onClick}
        tagSetData={activefilters}
        type="filter"
      />

  </...>

[EdwinGuzman]

Here's another idea, the onClick function returns two arguments, the first is the entire active tags and the second is the id of the tag that was just clicked to remove. There's still the issue with what do we call the clear filters tag but what I want to do is be able to support multiple cases. This makes sense in the RC context but it might be possible that another team would prefer the id of the tag that was clicked to remove. This way, we can return the same information but in different ways. It's more verbose but covers more cases.

const activefilters = [{id:"1"}, {id:"2"}, {id:"3"}, {id:"4"}, {id:"5"}];
const onClick = (activeTags, clickedTagId) => {
  // if the tag with id 3 is clicked, then
  // activeTags=[{id:"1"}, {id:"2"}, {id:"4"}, {id:"5"}]
  // clickedTagId="3"

   // if the "Clear Filters" button was clicked, then
  // activeTags=[]
  // clickedTagId="clear-filters"

  // now you do whatever you need to do
};

// ...
render (
  <... >

    <TagSet
        isDismissible
        onClick={onClick}
        tagSetData={activefilters}
        type="filter"
      />

  </...>

[charmingduchess]

Yea I agree that we shouldn't let the specific use case of the RC determine what the behavior should be. I like the idea of providing two arguments, but that leads back to the issue of clear filters passing an argument that needs special handling. I'm also wondering if maybe another solution to this is to have an onClick and onClear as two separate callbacks.
On the other hand, I also think in the context of filters, whichever filter is removed is of less importance than what filters remain. I don't immediately see a use case for the removed filter's data.

[EdwinGuzman]

I like the idea of breaking it up to two functions to make it clearer. The onClear just won't return anything though, it'll just clear the internal state. To me, it doesn't make sense to have it return an empty array since that's the expected behavior.

[bigfishdesign13]

I think it is important to know which tag was clicked and therefore removed. I can imagine a number fo use cases where this would a useful detail. And if TagSet only returns a list of the currently active tags, then the consuming app would need to jump through some hoops to compare the returned list of active tags with an existing list of active tags to determine which tag was now missing.

As for onClick vs. onClear, in this component, all click actions are clear actions. It's just that one button explicitly shows the word "clear." I am more in favor of having one function. It might be worth doing a quick audit to see where we are currently using onClick and onClear.

[EdwinGuzman]

I updated to return the clicked tag that was removed. Check out the updated working example in storybook for the action and the code and let me know what you think @bigfishdesign13 @charmingduchess https://nypl-design-system-git-dsd-1708-tagset-callback-value-nypl.vercel.app/?path=%2Fdocs%2Fcomponents-form-elements-tagset--docs

[bigfishdesign13]

This looks good to me, but I will alllow @charmingduchess to give the final approval here. I just had one concern about how you refer to one of the props.

[bigfishdesign13]

The onClick prop for the "filter" variant has a ?, so it is optional.

[EdwinGuzman]

This was always optional. The onClick prop is only applicable when isDismissible is true.

[bigfishdesign13]

But you say onClick is required. Did you mean it is required when isDismissible is true?

[EdwinGuzman]

Ah, I see. This wasn't updated in this pr but I'll update it.

[bigfishdesign13]

Oh, you're right. Sorry about that. Thanks for making the update.

[charmingduchess]

LGTM!