-
Notifications
You must be signed in to change notification settings - Fork 486
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes #1467 - Send useful filter label item data through components to Management Toolbar #1470
Fixes #1467 - Send useful filter label item data through components to Management Toolbar #1470
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor changes... biggest one, I don't think we can make a solid API with label values. I think we should rely on the whole object
<div class="null"><span class="label label-dismissible component-label tbar-label"><span class="label-item label-item-expand"><div class="label-section">Category: Label 3</div></span><span class="label-item label-item-after"><button class="btn close"aria-label="close"type="button"><svg class="lexicon-icon lexicon-icon-times"focusable="false"role="presentation"><use xlink:href="icons.svg#times" /></svg></button></span></span></div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This snapshot seems weird :D
<li class="tbar-item"><div class="tbar-section"><div class="null"><span class="label label-dismissible component-label tbar-label"><span class="label-item label-item-expand"><div class="label-section">Label 1</div></span><span class="label-item label-item-after"><button class="btn close"aria-label="close"type="button"><svg class="lexicon-icon lexicon-icon-times"focusable="false"role="presentation"><use xlink:href="spritemap.svg#times" /></svg></button></span></span></div></div></li><li class="tbar-item tbar-item-expand"><div class="tbar-section"><div class="null"><span class="label label-dismissible component-label tbar-label"><span class="label-item label-item-expand"><div class="label-section">Label 2</div></span><span class="label-item label-item-after"><button class="btn close"aria-label="close"type="button"><svg class="lexicon-icon lexicon-icon-times"focusable="false"role="presentation"><use xlink:href="spritemap.svg#times" /></svg></button></span></span></div></div></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here too, see div class="null"
@@ -37,6 +37,9 @@ class ClayResultsBar extends ClayComponent { | |||
*/ | |||
_handleFilterLabelCloseClicked(event) { | |||
return !this.emit({ | |||
data: { | |||
label: this.filterLabels[event.target.data.labelId], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we will likely need more than the label value, as that can be localized or who know what else. We would be better off passing down the id
, the whole list of label
and any additional information we might need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is already the entire label object.
Already approved by @jbalsas. Merging. |
No description provided.