feat(CompoundLabel) Add CompoundLabel component #464

PanSpagetka · 2018-07-09T13:40:46Z

In this PR I am creating component for showing key:values pair with delete capability. It can be used for filters or tagging

terezanovotna · 2018-07-09T14:30:36Z

Looks great! This is what I imagined, great job! 🎉🎉🎉

Interested in hearing what others think about it!
@mcarrano

jeff-phillips-18 · 2018-07-09T17:39:38Z

packages/patternfly-react/less/compound-label.less

@@ -0,0 +1,42 @@
+@import 'patternfly/variables';


It is assumed that this is already imported (for any of our less files to work).

jeff-phillips-18 · 2018-07-09T17:40:51Z

packages/patternfly-react/less/compound-label.less

@@ -0,0 +1,42 @@
+@import 'patternfly/variables';
+
+.pf-remove-button {


These selectors should have a compound-label-pf or similar prefix so as not to mistakenly override.

jeff-phillips-18 · 2018-07-09T17:42:09Z

packages/patternfly-react/src/components/Label/CompoundLabel.js

+      <Tooltip id="tooltip">{this.props.category.label}</Tooltip>
+    );
+    return (
+      <ul className="category list-inline">


We should allow callers to add a className

Do we want to pass along any other props?

jeff-phillips-18 · 2018-07-09T17:44:44Z

packages/patternfly-react/sass/patternfly-react/_compound-label.scss

+
+.category.list-inline {
+  background-color: $color-pf-blue-500;
+  padding: 3px 10px 3px 15px !important;


Shouldn't need the !important

jeff-phillips-18 · 2018-07-09T17:47:12Z

packages/patternfly-react/src/components/Label/CompoundLabel.js

+      <ul className="category list-inline">
+        <OverlayTrigger placement="bottom" overlay={categoryTooltip}>
+          <div className="category-label">
+            {this.props.categoryTruncate(this.props.category.label)}


Do we need to use the truncate function? Could we use a max-width and text-overflow instead?

I think having truncate function is more flexible and if tooltip should be shown only when text is truncated as @mcarrano suggests, I think it will be easier to implement if I stick to truncate function.

jeff-phillips-18 · 2018-07-09T17:47:55Z

packages/patternfly-react/src/components/Label/CompoundLabel.js

+
+CompoundLabel.propTypes = {
+  category: PropTypes.shape({
+    id: PropTypes.any.isRequired,


Please add comments so the descriptions show up in storybook.

jeff-phillips-18 · 2018-07-09T17:52:07Z

packages/patternfly-react/src/components/Label/CompoundLabel.js

+
+  render() {
+    const values = [...this.props.values];
+    if (values.length === 0) return <div />;


return null would be better.

any reason to return an empty div?

No, sorry, I missed this comment, null will be better

jeff-phillips-18 · 2018-07-09T17:53:47Z

packages/patternfly-react/src/components/Label/Label.stories.js

+      </div>
+    )
+  })
+);


Looks like the storybook was built before this was set correctly.

mcarrano · 2018-07-09T19:13:26Z

In general, I think this looks good. I'm still not sure that we need tooltips on a label if it is not truncated. What do you think @terezanovotna . The only other issue I see is that there appears to be a difference in the spacing of the internal chip with it's container (i.e. top and bottom margin not equal). Would be good to get some visual design input here.

terezanovotna · 2018-07-10T08:29:29Z

Thanks for the feedback @mcarrano.

When the compound label is not truncated, I don't think we need a tooltip. Can we do that @PanSpagetka ?

@kbaker Could you take a look on the storybook? Wondering what your visual perspective feedback is.

jeff-phillips-18 · 2018-07-10T10:16:49Z

I don't know of any other instances where we show a tooltip only when the text is truncated.

PanSpagetka · 2018-07-10T10:49:54Z

@jeff-phillips-18 thanks for review, I hope your concern are resolved in second commit. Except truncate function, which I would rather leave as it is.

dtaylor113 · 2018-07-10T16:00:25Z

Hi @PanSpagetka,
CompoundLabels look good!

Looks like the title in the Storybook should be 'Compound Label":

Also, the label & sub-labels do not wrap as expected at mobile sizes. As you can see above, the category label wraps within the dark blue container, and the the sub-labels are truncated. Maybe sub-labels should wrap within the blue container?
Here's what it looks like at mobile in angular-patternlfy:

Wondering how two <CompoundLabels/> wrap in relationship with each other at mobile?
-Thanks

jgiardino · 2018-07-10T20:15:32Z

Thanks for adding this!

I thought I had included this link in issue #401 but I failed to paste the correct link (I fixed it now):
https://codepen.io/jenngiardino/pen/XYzLwB

This is a codepen that I had worked on, which reuses much of the styles for the current label component, with a few minor CSS updates to extend it for this component.

Let me know if you think this is usable for this component or if you have any questions about this.

jgiardino · 2018-07-10T20:17:06Z

oh, fyi, @kybaker is out this week. It would be good to get his input on styles for this.

PanSpagetka · 2018-07-11T13:00:47Z

@dtaylor113 Yep, I agree, when it is in too narrow window, it looks bad. But I think when there are long labels in narrow space it would always feel bad. But if you think that inner sub-labels should wrap same as outer label, it could be done.

Multiple <CompoundLabels/> will wrap depending on the container there are in. It could be used for one label one row situation, like on your picture:

Or more labels next to each other like this:

Or maybe in some more crazy ways if needed, but I didn't test it.

@jgiardino Thanks, I think styles I am using now are good (maybe except issues with narrow resolutions) but if you agree that CompoundLabel should use different styles, I will be happy to use them.

jgiardino · 2018-07-11T18:31:20Z

@PanSpagetka
There are a couple of concerns I have with the html/css that's currently included:

The parent <ul> element includes <div> as an immediate child. While it does render, it is considered incorrect to have any element but <li> as a child. Once you make this change, the css might need updates, too.
Whenever possible, I prefer to reuse classes and styles that are already defined, instead of define new classes and styles. This helps to simplify maintenance of components. Since the Compound Label component is very similar to the simpler Label component, we should use the same classes and styles that are already defined for colors and spacing. This was an approach I was taking in the codepen I shared above.

I'm definitely open to suggestions on this. I also haven't fully tested the codepen above based on @dtaylor113's comments, so it's possible that it would need adjustments anyway. I was mainly testing what our options are for reusing existing classes and styles.

Also, it might be nice to allow the background color to be defined using props. The examples included in the design doc included above are different than what I see on patternfly.org. Maybe allowing the class names (e.g. label-primary, label-info) to be defined via props would allow this component to be more flexible in supporting different design requirements?

dtaylor113 · 2018-07-31T13:33:13Z

Hi @PanSpagetka, wondering what the status of the PR is? I'm looking to use the <CompoundLabel/> in a new Filter Panel (Checkbox Filter) component. -thanks

PanSpagetka · 2018-08-02T10:24:55Z

@dtaylor113 I have updated PR and storybook. State should be following:

Inner Labels should wrap when
Fixed wrong <li> and <ul> markup.
Allow to pass css classes to both components
Use more nested css styling

But if it is enough to be merged, I have no idea...

coveralls · 2018-08-02T10:45:33Z

Pull Request Test Coverage Report for Build 1911

14 of 16 (87.5%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.01%) to 77.6%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/patternfly-react/src/components/Label/CompoundLabel.js	9	10	90.0%
packages/patternfly-react/src/components/Label/LabelWithTooltip.js	5	6	83.33%

Totals
Change from base Build 1907:	-0.01%
Covered Lines:	2721
Relevant Lines:	3256

💛 - Coveralls

jeff-phillips-18 · 2018-08-02T13:53:21Z

@patternfly/patternfly-react-ux Could someone self-assign and do a UX review please?

jeff-phillips-18 · 2018-08-02T15:42:12Z

I agree with @jgiardino 's comments / codepen above. It would be great if we could go that route and keep styling changes to a minimum.

PanSpagetka · 2018-08-03T11:45:42Z

I can change styles for that @jgiardino posted but first I would like to be sure, that this is really what you want. Because some time ago, I received visual design review here and tried to change styling according to that.

But if you are sure, that this component should use styling proposed by @jgiardino it will use them. I just want to finish this and I really appreciate any advice which make it happened.

jgiardino · 2018-08-03T15:10:31Z

Thanks for sharing the feedback you received for this! I didn't realize there was a separate UI review outside of this repo.

The visual styling that I was trying to follow are the styles shown in the design document for Filters. My thinking was that this component should be general enough that it could be used for Filters as well as other use cases like the one in ManageIQ. I'd still love for that to be the goal for this PR, but I also understand needing to finish this up. And after reading the comments from the ManageIQ design review, the example I posted might require some updates to be able to accommodate things like the different font sizes that were requested.

Ideally, it would be great if the component allowed these customizations:

Consumer being able to define the background colors, which would be based on the classes already defined for the bootstrap Label component
Consumer being able to choose font sizes, based on the two cases we know of for now: ManageIQ and PatternFly Filters

But I feel like this could be a LOT of work at this time. And I'm also curious whether what I describe is even what our ultimate goal should be, and if so, is there a way that we can split this work out that makes sense.

@jeff-phillips-18 and @mcarrano, do you have any recommendations on how to move forward with this?

PanSpagetka · 2018-08-06T10:47:05Z

@jgiardino you can customize both, outer and inner component by passing them CSS classes using className="css-class" and innerClassName="another-css-class". You can also set bsStyle="warning" to make inner label to use particular styling for label. Setting innerClassName will override bsStyle.

The only problem is the color scheme proposed here is following: outer component has background-color: $color-pf-blue-500; and inner one background-color: $color-pf-blue-400; but there is no bsStyle with $color-pf-blue-400 so I have to set it with css class. But having it this way, setting 'bsStyle does not change background-color due to evaluation order.

I came up with three possible solutions:

Add new or modify existing bsStyle to use $color-pf-blue-400, it would solve the problem and I think it is the best solution
Change of inner label to some existing bsStyle color.
I could probably hack it somehow, to default color be $color-pf-blue-400 and still it being possible to customize it via bsStyle and custom css classes, but it could cause maintenance issues later.

I think each of these solution shouldn't take too much to to implement.

serenamarie125 · 2018-08-06T11:31:55Z

FYI those color recommendations that I provided in the other PR were from Jenny. @jgiardino, if you think these conflict, we can change as needed

jgiardino · 2018-08-06T20:44:49Z

I had a chat with @jeff-phillips-18, @mcarrano, and @kybaker

The conclusion regarding visual styling was to keep the color scheme the way Jenny suggested. (And to also update the filters design document to reuse the same color scheme.)

Also, @kybaker had suggested that the spacing be updated to make the left/right edges more even with the spacing at the top/bottom edges, but I think a designer should confirm whether that update makes sense for the ManageIQ use case before making that change here.

For the css class names that are added, we do ask that “-pf” is added somewhere in the name.

For the html structure, I would expect an html structure to present the entire compound label as a list item, with the list of values presented as a nested list inside that list item, similar to what is shown in this codepen. To be semantically accurate, I wouldn’t define the category label as a list item the way it is now, it’s really the label for the list of values.

Additionally, there will be updates needed to be able to use this component for Filters, based on quick testing I did using dev tools in the browser.

But I realize that these last two items can be significant undertakings. Aside from the css class names, I’m open to pushing further updates off to a later PR, assuming that we can update the html structure later without making breaking changes to the component. I’m guessing if the component name and properties are the same, then changes to the html tags are fine?

@jeff-phillips-18 Please weigh in with your thoughts on this one.

jeff-phillips-18 · 2018-08-07T10:42:12Z

Agreed, the html structure could change in a future PR as long as we can keep the public interface from changing. I was thinking compound-label-pf as the top level CSS class name.

PanSpagetka · 2018-08-07T11:42:55Z

@jgiardino @jeff-phillips-18 Thank you for your feedback! Ehm, I already made html changes and tried to use your css with some changes, to make it look similar. I think it does make sense to outer label be also bold, when inner labels are bold. But I could revert these changes if needed.

Old:

New:

I have added "-pf" postfix to first level css classes, but not to the nested ones, I think this should be ok?
Is there anything else which prevent this PR from merging?

jeff-phillips-18 · 2018-08-07T14:59:49Z

Once the ux-review label is changed to ux-approved I will re-review the PR.

jgiardino · 2018-08-07T18:59:10Z

Thanks for updating the html structure. The way you handled the css class names looks correct to me.

I'll let one of the designers comment on the expectations regarding what should be bold or not for the ManageIQ use case. (@terezanovotna are you the right designer to weigh in on this one?)

As for the use case of reusing this component for Filters, I'm assuming we can handle any further changes for that use case in a separate PR (e.g. like add a prop that will alter the visual styling when using as assigned tags vs filters).

As for css, I think the only change you need to make is to remove the float: left; from .compound-label-pf. Since this element is display: inline-block;, then you shouldn't need the float: left; (but please test this and make sure I'm right 😉).

PanSpagetka · 2018-08-08T08:35:31Z

@jgiardino Thanks, yep, it works. I have also deleted duplicate margin-bottom: 5px; from compound-label-pf class.

terezanovotna · 2018-08-09T12:46:55Z

@PanSpagetka can you add 5px margin to the left from "x"? At this moment it seems very close to the text.

jgiardino · 2018-08-09T12:51:08Z

@terezanovotna - I'd consider this change to be out of scope for this PR. The spacing for the X is based on the Label component css. If we change that, the change should be introduced in the pf core css repo, and not here.

@PanSpagetka can you add 5px margin to the left from "x"? At this moment it seems very close to the text.

terezanovotna · 2018-08-09T12:55:43Z

In that case LGTM!

@miq-bot remove_label ux/review
@miq-bot add_label ux/approved

We have a call about compound label pattern tomorrow, so if anything changes, I'll keep you updated @PanSpagetka

jeff-phillips-18 · 2018-08-14T18:28:49Z

packages/patternfly-react/src/components/Label/LabelWithTooltip.js

+  truncate: PropTypes.func.isRequired,
+  className: PropTypes.string,
+  bsStyle: PropTypes.string
+};


please add comments so descriptions show in storybook

jeff-phillips-18 · 2018-08-14T18:30:57Z

packages/patternfly-react/src/components/Label/LabelWithTooltip.js

+  className
+}) => (
+  <li key={value.id}>
+    <OverlayTrigger placement="bottom" overlay={tooltip(value.label)}>


the placement setting should be overridable via properties.

jeff-phillips-18 · 2018-08-14T18:31:13Z

packages/patternfly-react/src/components/Label/CompoundLabel.js

+    );
+    return (
+      <span className="label label-primary compound-label-pf">
+        <OverlayTrigger placement="bottom" overlay={categoryTooltip}>


the placement setting should be overridable via properties.

jeff-phillips-18 · 2018-08-14T18:32:57Z

packages/patternfly-react/src/components/Label/CompoundLabel.js

+      truncate={this.props.valueTruncate}
+      bsStyle={this.props.bsStyle}
+      className={this.props.innerClassName}
+    />


pass along overlay trigger placement here.

PanSpagetka · 2018-08-15T11:02:12Z

@jeff-phillips-18 I hope I have fixed issues you have mention.

PanSpagetka force-pushed the compound-label branch 2 times, most recently from 30bc880 to a6ea70f Compare July 9, 2018 14:28

terezanovotna mentioned this pull request Jul 9, 2018

Add/Modify Tag & Assigned Tags patternfly/patternfly-design#680

Closed

jeff-phillips-18 requested changes Jul 9, 2018

View reviewed changes

jeff-phillips-18 added the ux review label Jul 9, 2018

PanSpagetka force-pushed the compound-label branch from 9b230b5 to 8a9cc60 Compare August 2, 2018 10:38

terezanovotna assigned terezanovotna and unassigned terezanovotna Aug 6, 2018

terezanovotna self-requested a review August 6, 2018 07:25

PanSpagetka force-pushed the compound-label branch from 6d7c0bb to c7f0ef8 Compare August 7, 2018 11:42

terezanovotna previously approved these changes Aug 9, 2018

View reviewed changes

terezanovotna added ux approved and removed ux review labels Aug 10, 2018

jgiardino added the css approved label Aug 10, 2018

jeff-phillips-18 requested changes Aug 14, 2018

View reviewed changes

PanSpagetka dismissed terezanovotna’s stale review via 733e5ac August 15, 2018 10:59

PanSpagetka force-pushed the compound-label branch from 733e5ac to 8562a16 Compare August 15, 2018 11:29

PanSpagetka added 10 commits August 15, 2018 13:30

feat(CompoundLabel) Add CompoundLabel component

d3eed46

Address code review issues

2a42bc1

Update snapshots

f1f830c

Use separate templates for each storybook instead of default

18d53da

Fix markup, line wrapping, allow className to be passed

6383526

Changes styles, html structure

175680d

Remove float: left; and margin-bottom: 5px; from compound-label-pf class

6249959

Add description and allow set overlayPlacement in props

8562a16

Return null if no values to display

ddeaac0

Update tests, use new yarn prettier rules

54c702a

jeff-phillips-18 approved these changes Aug 15, 2018

View reviewed changes

jeff-phillips-18 merged commit 65e8030 into patternfly:master Aug 15, 2018

This was referenced Aug 21, 2018

CompoundLabel #401

Closed

[WIP] feat(Tagging) Introduce Tagging React component #351

Closed

		@@ -0,0 +1,42 @@
		@import 'patternfly/variables';

		.pf-remove-button {

feat(CompoundLabel) Add CompoundLabel component #464

feat(CompoundLabel) Add CompoundLabel component #464

Conversation

PanSpagetka commented Jul 9, 2018

Issue

Google design doc

Storybook

terezanovotna commented Jul 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcarrano commented Jul 9, 2018

terezanovotna commented Jul 10, 2018

jeff-phillips-18 commented Jul 10, 2018

PanSpagetka commented Jul 10, 2018

dtaylor113 commented Jul 10, 2018

jgiardino commented Jul 10, 2018

jgiardino commented Jul 10, 2018

PanSpagetka commented Jul 11, 2018 • edited Loading

jgiardino commented Jul 11, 2018

dtaylor113 commented Jul 31, 2018

PanSpagetka commented Aug 2, 2018

coveralls commented Aug 2, 2018 • edited Loading

Pull Request Test Coverage Report for Build 1911

💛 - Coveralls

jeff-phillips-18 commented Aug 2, 2018

jeff-phillips-18 commented Aug 2, 2018

PanSpagetka commented Aug 3, 2018

jgiardino commented Aug 3, 2018

PanSpagetka commented Aug 6, 2018

serenamarie125 commented Aug 6, 2018

jgiardino commented Aug 6, 2018

jeff-phillips-18 commented Aug 7, 2018

PanSpagetka commented Aug 7, 2018 • edited Loading

jeff-phillips-18 commented Aug 7, 2018

jgiardino commented Aug 7, 2018

PanSpagetka commented Aug 8, 2018

terezanovotna commented Aug 9, 2018

jgiardino commented Aug 9, 2018

terezanovotna commented Aug 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PanSpagetka commented Aug 15, 2018

PanSpagetka commented Jul 11, 2018 •

edited

Loading

coveralls commented Aug 2, 2018 •

edited

Loading

PanSpagetka commented Aug 7, 2018 •

edited

Loading