Use optgroup to distinguish between campaigns and topics #1053
Conversation
|
|
||
| if ( | ||
| this.campaignSelect.options[this.campaignSelect.selectedIndex] | ||
| .parentElement.label == 'Topics' |
There was a problem hiding this comment.
Think it's worth keying this test off of a class so it won't change if we touch the UI labels?
There was a problem hiding this comment.
Reviewing this is making me think that we should rename the campaign selector to a less-specific name now that it's doing double-duty but am not coming up with non-awkward generic language. Maybe something like “containerFilter”?
I hear you on this, but I think we should wait until we see what happens next and whether we're going to refactor Campaigns/Topics so that the whole design is cleaner.
| @@ -839,6 +882,13 @@ export class ActionApp { | |||
| continue; | |||
| } | |||
|
|
|||
| if ( | |||
| currentTopicId && | |||
| !this.assetHasTopic(asset, currentTopicId) | |||
There was a problem hiding this comment.
I'm wondering whether this should simply be !asset.topics.contains(currentTopicId)
There was a problem hiding this comment.
Only if I flatten asset.topics to a plain array of just IDs - it's currently an array of dictionaries.
There was a problem hiding this comment.
Ah, right – I hadn't gotten that far in the code yet when I left that comment. It might be nice to avoid a check-at-a-distance but I'm not sure that's worth changing topics into a dictionary.
| // The values specified in API responses are integers, not DOM strings: | ||
| currentCampaignId = parseInt(currentCampaignId, 10); |
There was a problem hiding this comment.
We could remove this block by converting currentCampaignSelectValue first so there's only one parseInt call.
There was a problem hiding this comment.
I can de-dupe the parseInt call but I only want currentCampaignId to be defined if it's a campaign that was selected and not a topic.
There was a problem hiding this comment.
The de-dupe is what I was thinking about. Leaving the variables is a little busy but the ways to refactor that would either take repeated extra string comparisons against campaignOrTopic, function calls, etc. and I'd prefer not to tackle something bigger yet.
| if ( | ||
| this.campaignSelect.options[ | ||
| this.campaignSelect.selectedIndex | ||
| ].parentElement.classList.contains('topic-optgroup') |
There was a problem hiding this comment.
I'd probably favor a class name which doesn't describe the tag (e.g. topics) but I'm not sure it's that big a deal.
|
Okay, I'm going to merge this and test in crowd-dev, then prep the release and test in crowd-stage. If all goes well it's going out by COB today. |
Use optgroup to distinguish between campaigns and topics in activity UI dropdown.