Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use optgroup to distinguish between campaigns and topics #1053

Merged
merged 4 commits into from
Jun 5, 2019
Merged

Use optgroup to distinguish between campaigns and topics #1053

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
efb44a4
Use optgroup to distinguish between campaigns and topics in activity …
rstorey Jun 5, 2019
fd79583
Asset metadata topics is a list of dictionaries
rstorey Jun 5, 2019
8cc1ce9
use class list instead of label to determine whether it's a campaign …
rstorey Jun 5, 2019
f15d0e1
Bug fix for determining whether asset has a given topic
rstorey Jun 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 62 additions & 6 deletions concordia/static/js/action-app/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,11 @@ export class ActionApp {

addToState(key, value) {
this.persistentState.set(key, value);
if (key == 'campaign') {
this.persistentState.delete('topic');
} else if (key == 'topic') {
this.persistentState.delete('campaign');
}
this.serializeStateToURL();
}

Expand Down Expand Up @@ -424,6 +429,9 @@ export class ActionApp {
this.campaignSelect = $('#selected-campaign');
fetchJSON(this.config.urls.campaignList)
.then(data => {
let campaignOptGroup = document.createElement('optgroup');
campaignOptGroup.label = 'Campaigns';
this.campaignSelect.appendChild(campaignOptGroup);
data.objects.forEach(campaign => {
let o = document.createElement('option');
o.value = campaign.id;
Expand All @@ -436,7 +444,7 @@ export class ActionApp {
}
);

this.campaignSelect.appendChild(o);
campaignOptGroup.appendChild(o);
});
})
.then(() => {
Expand All @@ -448,12 +456,18 @@ export class ActionApp {
});

this.campaignSelect.addEventListener('change', () => {
this.addToState('campaign', this.campaignSelect.value);
let campaignOrTopic = this.getSelectedOptionType();
this.addToState(campaignOrTopic, this.campaignSelect.value);
this.updateAssetList();
});

fetchJSON(this.config.urls.topicList)
.then(data => {
let topicOptGroup = document.createElement('optgroup');
topicOptGroup.label = 'Topics';
topicOptGroup.classList.add('topic-optgroup');
this.campaignSelect.appendChild(topicOptGroup);

data.objects.forEach(topic => {
let o = document.createElement('option');
o.value = topic.id;
Expand All @@ -466,7 +480,7 @@ export class ActionApp {
}
);

this.campaignSelect.appendChild(o);
topicOptGroup.appendChild(o);
});
})
.then(() => {
Expand Down Expand Up @@ -792,6 +806,29 @@ export class ActionApp {
return sortBy(assetList, keyFromAsset);
}

getSelectedOptionType() {
let campaignOrTopic = 'campaign';

if (
this.campaignSelect.options[
this.campaignSelect.selectedIndex
].parentElement.classList.contains('topic-optgroup')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

) {
campaignOrTopic = 'topic';
}

return campaignOrTopic;
}

assetHasTopic(asset, topicId) {
for (let idx in asset.topics) {
if (asset.topics[idx].id === topicId) {
return true;
}
}
return false;
}

getVisibleAssets() {
// We allow passing a list of asset IDs which should always be included
// to avoid jarring UI transitions (the display code is responsible for
Expand All @@ -807,10 +844,22 @@ export class ActionApp {
alwaysIncludedAssetIDs.add(this.persistentState.get('asset'));
}

let currentCampaignId = this.campaignSelect.value;
if (currentCampaignId) {
let currentCampaignSelectValue = this.campaignSelect.value;
let currentCampaignId;
let currentTopicId;
if (currentCampaignSelectValue) {
let campaignOrTopic = this.getSelectedOptionType();
currentCampaignSelectValue = parseInt(
currentCampaignSelectValue,
10
);

// The values specified in API responses are integers, not DOM strings:
currentCampaignId = parseInt(currentCampaignId, 10);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could remove this block by converting currentCampaignSelectValue first so there's only one parseInt call.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (campaignOrTopic == 'campaign') {
currentCampaignId = currentCampaignSelectValue;
} else {
currentTopicId = currentCampaignSelectValue;
}
}

let currentStatuses;
Expand Down Expand Up @@ -839,6 +888,13 @@ export class ActionApp {
continue;
}

if (
currentTopicId &&
!this.assetHasTopic(asset, currentTopicId)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering whether this should simply be !asset.topics.contains(currentTopicId)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only if I flatten asset.topics to a plain array of just IDs - it's currently an array of dictionaries.

Copy link
Member

@acdha acdha Jun 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

) {
continue;
}

if (!currentStatuses.includes(asset.status)) {
continue;
}
Expand Down
10 changes: 10 additions & 0 deletions concordia/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1717,6 +1717,16 @@ def serialize_object(self, obj):
},
}

if project.topics:
metadata["topics"] = []

for topic in project.topics.all():
new_topic = {}
new_topic["id"] = topic.pk
new_topic["title"] = topic.title
new_topic["url"] = topic.get_absolute_url()
metadata["topics"].append(new_topic)

# FIXME: we want to rework how this is done after deprecating Asset.media_url
if obj.previous_sequence:
metadata["previous_thumbnail"] = re.sub(
Expand Down