Skip to content

Store LevelGroup results on a single namespaced object#7874

Merged
joshlory merged 10 commits into
stagingfrom
level-group-results
Apr 15, 2016
Merged

Store LevelGroup results on a single namespaced object#7874
joshlory merged 10 commits into
stagingfrom
level-group-results

Conversation

@joshlory

Copy link
Copy Markdown
Contributor

Brent suggested storing individual level results on a single object in a new levelGroup namespace, instead of having multiple global vars.

This PR also includes an optional change to store LevelGroup attempts as a hash instead of an array.

Josh Lory added 7 commits April 13, 2016 17:44
#{last_attempt.to_json}
);
});
window.levelGroup.results[#{level.id}] = new Multi(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It seems weird to be assigning a Multi object into something called results, doesn't it?

@breville

Copy link
Copy Markdown
Member

thanks, this lgtm. Tagging @laurelfan as an FYI, since she's been reviewing everything related to LevelGroup and long assessments so far.

@breville

Copy link
Copy Markdown
Member

Oh, except main concern around assigning our level-behaviour objects into something called results. I think the name should probably be something closer to "level objects" but I'm not sure what offhand.

@joshlory

Copy link
Copy Markdown
Contributor Author

Hmm, maybe levelGroup.questions?

@joshlory

joshlory commented Apr 14, 2016

Copy link
Copy Markdown
Contributor Author

This PR is also waiting for feedback from @laurelfan / @ashercodeorg about whether switching to use the levelId as the key is a good idea (commit 0cd576d).

@ashercodeorg

Copy link
Copy Markdown
Contributor

SGTM (changing to use levelId as the key), I don't have a preference.

@joshlory joshlory force-pushed the level-group-results branch 2 times, most recently from 89f3be2 to 32a6748 Compare April 14, 2016 22:18
- if index # If an index was provided we're inside a LevelGroup.
:javascript
window['level_#{index}'] = {
window.levelGroup.questions[#{level.id}] = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is this questions or answers?

@joshlory joshlory Apr 14, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I originally named it results but Brendan suggested I call it something else: #7874 (comment). It's the collection of Multi/TextMatch/FreeResponse objects that provide a getCurrentAnswer method.

Open to suggestions for a better name!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hm, I was thinking levels (since a levelgroup is made out of levels in most places)... or is that name too overloaded?

@joshlory joshlory merged commit 837bac8 into staging Apr 15, 2016
@joshlory joshlory deleted the level-group-results branch April 15, 2016 01:28
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.

4 participants