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.

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

Re-add filtering to song display #25897

Merged
merged 6 commits into from Nov 8, 2018
Merged

Re-add filtering to song display #25897

merged 6 commits into from Nov 8, 2018

Conversation

epeach
Copy link

@epeach epeach commented Nov 7, 2018

Issue: code-dot-org/dance-party#169

Filtering code didn't make it through recent refactor of song display. Re-adding filter check.

Failure wasn't caught by age_filter tests as a known issue was causing the tests to fail and I didn't look beyond the known issue to discover this error. Age_filter tests should be fixed now, so any failures should indicate regression.

@@ -37,7 +37,9 @@ const SongSelector = Radium(class extends React.Component {
<label><b>{i18n.selectSong()}</b></label>
<select id="song_selector" style={styles.selectStyle} onChange={this.changeSong} value={this.props.selectedSong}>
{Object.keys(this.props.songData).map((option, i) => (
<option key={i} value={option}>{this.props.songData[option].title}</option>
((this.props.songData[option].pg13 && !this.props.filterOff) || !this.props.songData[option].pg13) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be simplified to

this.props.filterOff || this.props.songData[option].pg13

let over13 = parseInt(value, 10) >= 13;
sessionStorage.setItem(sessionStorageKey, over13);
if (over13) {
reload();
Copy link
Author

@epeach epeach Nov 7, 2018

Choose a reason for hiding this comment

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

@Bjvanminnen When a logged out user goes to a dance level or project, this dialog is displayed over the dance level (so the level is already loaded behind the dialog), so we somehow need to trigger an update. My naive approach is to just refresh, but we probably don't want that to happen at a wide scale. Any ideas?

Copy link
Contributor

Choose a reason for hiding this comment

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

I seem to remember running into some similar problems to this last year, where we did just go with a reload.

One option could be to just say that even if you're over 13, you won't see the songs the first time (but will when you advance to the next puzzle). This might be good enough (but may not be). We'd need to check with PM.

If not good enough, we need to figure out what all would need to happen to get the songs. I'm not super familiar with the loading/filtering pathway, but I could conceive of a scenario where we always load all the songs, and then just toggle which we show based on some state we have in redux. Then we wouldn't need to do anything, as everything would update appropriately as soon as we set our state.

@epeach
Copy link
Author

epeach commented Nov 7, 2018

@Bjvanminnen PTAL - thanks! I ended up finding and closing some other loopholes while editing the boolean as suggested. Could use input on the refresh case.

Copy link
Contributor

@Bjvanminnen Bjvanminnen left a comment

Choose a reason for hiding this comment

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

existing changes lgtm

@epeach epeach merged commit 88c65ff into staging Nov 8, 2018
@epeach epeach mentioned this pull request Nov 9, 2018
@epeach epeach deleted the age-restriction-dialog branch November 13, 2018 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants