-
-
Notifications
You must be signed in to change notification settings - Fork 166
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
Move achievement item use form to ProblemSet page. #2664
Move achievement item use form to ProblemSet page. #2664
Conversation
I made some typo suggested edits (which you could ignore in GH and just fold into your next commit if you like). And I left some other comments about certain wording. I haven't done a comprehensive review of all the wordings. This will be better than what we have now. I think this came up in discussion at a recent meeting, do I remember right? I think on the Achievements page, the Reward item titles no longer need the Now there are three buttons:
I wonder if it would look better (in a wide enough screen) like:
Perhaps beyond the scope of this PR, but if those buttons became icons and were placed more near the top of the page, I think it would be better. That includes "Email Instructor" on other pages besides a problem page. Maybe this is just my opinion. |
e81a3a3
to
e9c4625
Compare
@Alex-Jordan thanks, I rebased the branch onto develop and updated all the typos. |
e9c4625
to
4ca6bb5
Compare
@Alex-Jordan I updated a bunch of your suggestions. Leaving the button layout alone for now. If it is agreed to change I can wrap it in here, or it can be a future PR. |
4f90e1b
to
9ac8de3
Compare
I believe all current achievements reward items are something you apply to a set. (Possibly to a problem or problems within a set.) Would it make sense to ever have an achievement reward item that does not apply to a particular set? Like a reward item where across the course, all exercises where you earned partial credit get elevated to 100%. Or an item that reopens all sets but reduced credit is in effect. Grant additional attempts to all problems across the course. Etc. If such an item existed, how do you imagine giving students the ability to use it? Would it just always show up among the items in the modal dialog for the "Use Achievement Item" button in any set's problem list page? Or would such things be activatable from the Achievements page? Before we do away with the ability to activate these from the Achievements page, I wanted to ask if we are sure that would be OK. |
f8e5b6d
to
828f455
Compare
My original approach was to allow this 'Use Achievement Reward' button to be used on multiple pages, and the That being said, if things a need arises to allow using achievements from different pages, it should be setup to make adding that feature not too difficult, but I would think about that for future work. I just stuck to a single page for now for simplicity and currently we don't have such achievements. |
OK, it's good that however things are working for this, it is extensible to possible future situations. |
There is a problem with this pull request that needs to be fixed. If |
@drgrice1 thanks for catching that, should be fixed now. |
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 generally looks good. However, there seem to be several typos and inconsistency in the language for several of the rewards. Also there are few UI details that could use improvement.
Also, there is something odd with the Surprise
achievement. The oddity is in the develop branch as well, and somewhat remedied in this pull request, but not completely. You removed the submit button for the reward that doesn't do anything, but yet the item still shows Mysterious Package (with Ribbons) (n remaining)
on the achievements page even though there is no way to use it or decrease the number of remaining rewards in this case.
templates/ContentGenerator/ProblemSet/use_achievement_items.html.ep
Outdated
Show resolved
Hide resolved
templates/ContentGenerator/ProblemSet/use_achievement_items.html.ep
Outdated
Show resolved
Hide resolved
eb0a205
to
69d7aaf
Compare
@drgrice1 Thanks for the review. I have committed all your suggested changes and removed the number of remaining items from the remaining title of the Surprise achievement item. I am currently out of town so I am unable to test the changes, I just had some down time to commit them. I will test further when I return home next week. |
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 this is looking good now. I think this will be a much nicer way to use achievement rewards. Thanks for your work on this.
Move the forms to use achievement items to a modal on the problem set page. If any achievement item is usable on the current set, a button to "Use Achievement Reward" will be available, and open a modal which lists all achievement items that can be applied. The Achievements page still lists all rewards the student has earned, and provides instructions to use them from the problem assignment they wish to use the achievement on.
Remove the fs-3 class from the achievement item names on the achievements page when listing the achievement items. When printing the achievement item name with the number of remaining items, don't treat separately from any > 0 value. Update the problem selection drop down forms to be clearer on what the problem number is for and clarifying when double means double the weight vs increase grade by 50%.
…ement items are disabled.
e8594e3
to
304886f
Compare
Move the achievement item use forms from the achievement page to the problem set page. The primary goal is to make less likely a student applies an achievement to the wrong set or problem when using drop down menus to select between multiple sets and problems on the achievements page, by placing the use item form on the set they wish to apply it to.
If achievement items are enabled, this adds a button to the bottom of the problem set page to "Use Achievement Reward" which opens a modal with a list of all available achievement items that can be used on the current set. Since the items now have access to the merged set and problem records, they can use this to both determine if the item
can be used, and include it in the form output. This makes the output more useful, stating what the extended due date is, or what the current and final grade would be when using an item.
If no achievement items are usable the button is disabled with a hover explaining why. Achievement items can also
no longer be used when acting as another user. In this case the button is also disabled. I decided to not setup a permission for this, because to me the items are for students, instructors can override the student data in other ways.
The achievement page now lists all earned items (including count), with a description that to use an item they must go to the set they wish to use it on and click the "Use Achievement Reward" button. When acting as a user this list shows up as well, so instructors can see what achievements a student still has available but not use them.
This is built on top of #2523 so I could update all achievement items and test them. So that should be merged first.
I have modified the gateway achievements temporarily. Due to the new setup I didn't implement applying extensions to the test versions, now the achievements only apply extensions to the template set. This is partly due to #2646, with comments that a better setup needs to be done (my thought is only allow extending open test versions that end at the same time as the template test vs ones that have a timer set).
Here is an achievement to give you 15 of all achievement items to test. If you want to test the unlimited use, set the count to a negative number.