[WIP] feat(Tagging) Introduce Tagging React component

[PanSpagetka]

Add Tagging component in React to patternfly repo. Originally introduced in ManageIQ/react-ui-components#3

Link to Storybook:
https://PanSpagetka.github.io/patternfly-react/

Additional issues:

[jeff-phillips-18]

@PanSpagetka Thanks for contributing! Couple of quick comments.

Please use .js rather than .jsx for the react javascript files.
We need to support both less and sass, currently we do not have a converter (#169)

[jeff-phillips-18]

@patternfly/patternfly-react-ux Looking for a volunteer to self-assign themselves to handle the UX review for this

[PanSpagetka]

@jeff-phillips-18 Yep, still working on some issues with css build process, but I will add less version.

Storybook is working and I don't plan any major changes, so it is ready for UX review. Maybe I would change sorting of items in Selects and Categories/Values, but that could be subject of UX review.

[terezanovotna]

@jeff-phillips-18 Self-Assigning myself for UX Review!

[terezanovotna]

Hi @PanSpagetka, I have few things I've noticed.

1. message on hover needs to be displayed quicker. I know its there, but the user is not going to wait for it.

2. Displaying icon only once

3. I see that the tags are organized in the order the user added them. Could we organize them alphabetically?

4. Tag truncation - Do we want to show super long tags? We probably don't want to display super long tags, but what limitation should we have? When truncated, the rest of the name should appear on hover.

Great job so far, tags look great!!
@patternfly/patternfly-react-ux anything else you noticed?

[coveralls]

Pull Request Test Coverage Report for Build 1453

• 51 of 55 (92.73%) changed or added relevant lines in 9 files are covered.
• 23 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+0.7%) to 74.777%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/tagging/src/components/InnerComponents/Tag.jsx	3	4	75.0%
packages/tagging/src/components/InnerComponents/TagView.jsx	5	6	83.33%
packages/tagging/src/components/InnerComponents/ValueSelector.jsx	4	5	80.0%
packages/tagging/src/components/Tagging/Tagging.jsx	11	12	91.67%

Files with Coverage Reduction	New Missed Lines	%
packages/console/src/SerialConsole/XTerm.js	23	17.74%

Totals
Change from base Build 1359:	0.7%
Covered Lines:	1768
Relevant Lines:	2158

---

💛 - Coveralls

[terezanovotna]

Is the storybook link showing the latest changes @PanSpagetka? Or am I being too proactive?

[PanSpagetka]

@terezanovotna I forgot to update it, now is https://PanSpagetka.github.io/patternfly-react/ running new version

[terezanovotna]

Looks great!!

Can we make the hover faster?

• "i" it takes a while to figure out there's a hover on the i

- I like how we truncated both parts of the tag. Can we make the hover appear faster on both areas?

[PanSpagetka]

@terezanovotna time before hover appears should be same as everywhere else. I believe, that it is browser dependent. There is no easy, non-hacking way to do it, so I would like to leave like it is now.

[jgiardino]

Hi @PanSpagetka, thanks for opening this PR!

Is this contribution providing the UI shown under the heading "Assigned Tags", or does it also include the UI shown under the heading "Add/Modify Tag"? If it does include the UI shown under "Add/Modify Tag" then we would need to work with the patternfly design team, as noted here in contributing.md (5th bullet).

Most storybooks include a link to the design documentation for the component as well as a button to view the storybook code, but this section of the storybook is missing.

[PanSpagetka]

Hey @jgiardino

It is providing both parts, "Assigned Tags" and "Add/Modify Tag" one. But it is exporting only static component for both parts. Interaction is separate wrapper component, which is not exported and serves as an example for storybook.

I worked with @terezanovotna and we figure out this design, there might not be any kind of official documentation, but I am not sure. I believe, that more member of design team reviewed this in ManageIQ/react-ui-components#3

[jgiardino]

Hi @PanSpagetka,

I was chatting with @terezanovotna yesterday about this component. Currently, as part of our react component requirements, when components are contributed they should have existing documentation on patternfly.org. In cases where a component does not have design documentation, then the contributors can work with the patternfly designers to determine if the component, or some aspect of the component, passes the patternfly decision tree.

@terezanovotna is going to work with @mcarrano on this process for the part of the storybook that's labeled "Add/Modify Tags".

But the part that's labeled "Assigned Tags" already is very similar to our latest design for filter chips in our Filters design documentation. If you're interested, you could split this part out and contribute that piece, while we wait for the other part to go through the pf decision tree process.

[terezanovotna]

Thanks @jgiardino for the guidance. @PanSpagetka and I discussed what would be the best way to divide these components so they could be more generalized.

Here is a design pattern issue.

What do we have to do to move it forward?

Should we discuss it on a quick call? @PanSpagetka @jgiardino @mcarrano
Thanks!

[mcarrano]

I agree with @jgiardino . The only thing that is really unique here is the Tags themselves and this is really an elaboration on the existing filter chips. The remainder of this can be built from existing components and is best documented as a design pattern to ensure a consistent approach across products. @PanSpagetka @terezanovotna is that also in line with your thinking?

As an additional point of information, for PF4, we identified the need for a more generalized Tag component that can satisfy use cases like this as well as filtering.

[karelhala]

@jgiardino great points! However I don't think this falls under react component requirements since this component does not have PF design yet and the sole purpose of this being here is for others to use this component. Based on the decision tree (btw nice treee, didn't know about it) it passes it rather easilly. Once UX team brings design for this component I guess @PanSpagetka will be happy to rewrite it based on the design. But I don't think it needs to hold this PR from merging, correct me if I am wrong but when I was discussing Monorepo with @ohadlevy and @priley86 I got and impression that we would be able to introduce new components which does not directly fall under PF design but we feel like that it might be of interest for other teams.

Btw somewhat similiar discussion was when SerialConsole was introduced but at that time we didn't had the luxury of monorepo.

We have two options here:

1. Merge this without PF design and let PF team to work on it without them to work in a hurry.
2. Wait for PF design on this component and in the meantime create new component in side repository and once such design is provided rewrite everything.

For me personally it feels like second option is much more work, but it will be easier for @PanSpagetka to make this fork for ManageIq. But we'll loose the option to be shared amongst other projects, which is kinda a bummer.

@mcarrano Sadly this is not applicable to filter chips since the both categories can have a lot of values and user needs to both filter trough them to make the work much easier. In theory it serves to same purpose so with some minor tweaks filter chips could be used for this, but these 2 designs aim to different use cases and should bring users to different thinking. One to filter trough some content while the second adds some properties to content.

However if any of you think that this shouldn't be part of PF react repository no big deal we are glad that designers started to talk about this and will come up with much better design. In the meantime we will use this component in our repo and once solid design is introduced we'll use it.

[jgiardino]

Thanks for sharing your thoughts, @karelhala! A lot of what you say is totally valid.

I have also heard that after the changes we made related to the monorepo work, we could enable contributors to share code that hasn't been fully vetted by the patternfly design review process. And that these components would be available in a separate package from the core patternfly-react package. I don't know if there was a final decision on that yet, so I'm adding @dgutride and @priley86 in case they have any more information to share.

I would still recommend that the part of this contribution that's labeled "Assigned Tags", shown here:

be contributed in a way that it can serve for both the original requirement in ManageIQ as well as the pattern documented for Filter Chips, shown here:

This tag/label/filter chip component seems general enough that it could be used in both patterns. And if we can create a component that could serve both purposes, and for which a pf design already exists, then I'd suggest we introduce that piece to the core patternfly-react package of this repo. We definitely need a component like this.

@mcarrano Sadly this is not applicable to filter chips since the both categories can have a lot of values and user needs to both filter trough them to make the work much easier. In theory it serves to same purpose so with some minor tweaks filter chips could be used for this, but these 2 designs aim to different use cases and should bring users to different thinking. One to filter trough some content while the second adds some properties to content.

The example I included above is quite new to the patternfly design library. Do you think this new pattern would work for tags? I think if the same visual design can be used for both, then the same component can be used for both, even if they are supporting different use cases.

In @mcarrano's comment:

The remainder of this can be built from existing components and is best documented as a design pattern to ensure a consistent approach across products.

I agree with the idea of constructing this design from separate, independent components. At the moment, we don't have a Select component. But it would be great to have the pieces of this as separate components that can be used independently.

[PanSpagetka]

@jgiardino I agree that we can use chips for both, Tagging and Filtering. There exist Filter component in patternfly-react core package with simple version of these chips.

I try to wrap up what you are suggesting, correct me if I am wrong.

• Extract chips from Tagging and Filter into separate Chips component, with grouped and simple version.
• Use these Chips in Tagging and Filter
• Use PatternflySelect in Tagging when we have one

So I would do a separate PR extracting 'Chips' and another one for integrating into Filter. In the meantime, can this PR be merged as a separate package or shall we wait until Chips component is made and integrated?

Is there another design/process problem with this approach?

[jgiardino]

@PanSpagetka The breakdown of components that you describe sounds right to me, but would love to have one of the JS devs confirm. @priley86, @jeff-phillips-18, or @mturley - do you have any thoughts on how to break these pieces down into individual components?

One thing I'm wondering is if what @PanSpagetka refers to as a Chip is just a variation of the Label? Should we extend the Label component to support the second example here, or would both examples below be included as a new and separate component from 'Label':

If it is a new component, does anyone have concerns with Chip or is there another name we should use instead? (adding @mcarrano in case he has thoughts on this one)

In the meantime, can this PR be merged as a separate package or shall we wait until Chips component is made and integrated?

I think this is possible, but would like guidance from the JS devs mentioned above on this.

[mcarrano]

After looking at this further, I agree with @jgiardino that what is being proposed here is really just an extension of the Label component and should be treated as such. I'd suggest we define a new variant that is called something like a Compound Label to describe a single attribute label with multiple values applied.

So while the use case/design pattern can be called "Tagging" it is actually using a Label component. I think the same would be true for what we are calling a Filter Chip. @terezanovotna please take note as you are writing up the design doc.

[PanSpagetka]

Yep this could be treated as extension of label component, but i still suggest making separate components not extend Label. So Label component will stay exactly same as it it now, just create variants of labels as a new components.

One we can call CompoundLabel and it will represent grouped Key:Value(s) pair displayed in Label way. Both Labels in picture will be CompoundLabels

Second one, I couldn't come with nice name, let's call it for now KeyValueLabel would represent simpler case, just one Key:Value pair displayed in Label. Even though technically is not necessary to make this new component, it is possible to use Label with key:value string, I would still do it because I want to separate Key and Value as a separate pieces of data.

[jeff-phillips-18]

The tagging component was completed by #464 as CompoundLabel.

[PanSpagetka]

@jeff-phillips-18 No, #464 is just one part of Tagging component, this PR is still relevant.

[jeff-phillips-18]

@PanSpagetka Ah, OK. Reopening.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.