feat(Label): add button for adding new labels#4828
Conversation
|
Preview: https://patternfly-pr-4828.surge.sh A11y report: https://patternfly-pr-4828-a11y.surge.sh |
mcarrano
left a comment
mcarrano
left a comment
There was a problem hiding this comment.
This looks good @thatblindgeye . Do you think we need an example that shows the Add label and overflow buttons existing together? Not sure, but wanted to raise the question.
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
Really great job @thatblindgeye!!! Just a few nits, and we discussed that the add button should (probably?) not be part of the label group list as it isn't an item in the group list - it's more like a control for the group. This will be inline with the add button we recently added to tabs, too.
| @@ -1,4 +1,4 @@ | |||
| <{{# if label--type}}{{label--type}}{{else if label--IsOverflow}}button{{else}}span{{/if}} class="pf-c-label{{#if label--IsOverflow}} pf-m-overflow{{/if}}{{#if label--modifier}} {{label--modifier}}{{/if}}{{#if label--IsEditable}} pf-m-editable{{/if}}{{#if label--IsEditableActive}} pf-m-editable-active{{/if}}" | |||
| <{{# if label--type}}{{label--type}}{{else if label--IsOverflow}}button{{else if label--IsAdd}}button{{else}}span{{/if}} class="pf-c-label{{#if label--IsOverflow}} pf-m-overflow{{/if}}{{#if label--IsAdd}} pf-m-add{{/if}}{{#if label--modifier}} {{label--modifier}}{{/if}}{{#if label--IsEditable}} pf-m-editable{{/if}}{{#if label--IsEditableActive}} pf-m-editable-active{{/if}}" | |||
| {{#if label--attribute}} | |||
There was a problem hiding this comment.
I just noticed this label is missing type="button" when it's a <button> element (default is type="submit" which we don't want). Could you add a conditional in here that tests for IsOverflow/Add and adds type="button"?
thatblindgeye
force-pushed
the
labelGroup_add-label
branch
from
d3c61c6 to
5a32fa7
Compare
@mcoker As mentioned in our convo, I'm leaning towards this being something for next milestone possibly? I know it was mentioned when we sat to discuss this that getting the add button to be more inline but also outside the label group list might be tricky, so it may just require a little more research/discussion. |
mcarrano
left a comment
mcarrano
left a comment
There was a problem hiding this comment.
Looks good. Thanks @thatblindgeye !
|
🎉 This PR is included in version 4.193.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
4 participants
Closes #4756
Convenience links:
Label componeont
LabelGroup component
This PR:
One thing to consider with how I updated the CSS is that within the Label page, the variables aren't exposed below the doc table for the
pf-m-addselector, and within the CSS itself this selector is also using variables with "overflow" in their name. I believe an alternative would be to copy+paste any styles for the overflow and change them to reflect this add label (--pf-c-label--m-add__content--Colorand so on). Since the overflow and add labels are both assumed to always share the same styles, though, I'm wondering whether if at some point down the line these variables would be updated to be less specific to the overflow (though this still doesn't resolve this possible issue in the present)?