-
Notifications
You must be signed in to change notification settings - Fork 60
Step Builder: Support for additional overlays for plot blocks #82
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
Conversation
2623aa5 to
4ee4532
Compare
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.
You're sure there's no way to use :hidden? This seems a bit redundant. If there isn't, then ok. Same question with isVisible down below.
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.
@Kelketek This is to ensure all edge cases can be handled properly while keeping toggling behavior of information for individual overlays transparent. By the latter I mean that we can keep the logic for showing overlay info simpler if we make sure that the display status of overlay info always corresponds to the display status of the corresponding overlay. If an overlay is on, associated info will be "visible" (as in, display: block); if it's off, associated info will be hidden (as in, display: none). This requirement alone does preclude using :hidden/:visible, but there are a couple more aspects to consider:
- The parent element that groups overlay info includes a header ("Plot info") that we want to show when info about one or more overlays is visible.
- Some overlays have neither a description nor a citation. In that case, we don't really have any info to show (plot label doesn't count since it's already used for the button that toggles the overlay).
- We would like to avoid showing the "Plot info" header if there is no info to display; i.e., if all overlays are either off or don't have any info associated with them.
So if we start out with all overlays off and then turn on a single overlay that doesn't have a description or a citation, we don't want to show the parent element. As a result, the parent element will have display: none, while the child will have display: block. At this point, we can't reliably use .is(":visible") on the child anymore to determine whether the child should be shown or not because if the parent is visible, it considers the child to be visible as well. This is why I'm not using :hidden/:visible here.
Writing this out now I realize that instead of toggling visibility of the parent, it would probably also be possible to keep the parent visible at all times and just toggle visibility of the "Plot info" header. In that case we could use :hidden/:visible here. However, I'm not sure if that would yield the cleanest solution in terms of what we do and don't show on the page at any given time: If we show the parent even if overlay info for all active overlays is "empty", the parent will end up taking some space on the page because the children are not really empty (they appear to be, but they do contain some whitespace). So yeah, overall I think I slightly favor the current approach, which could be summarized as "If we only have whitespace to show, we'd rather not show anything at all".
Let me know what you think; I'll hold off on merging for now just in case.
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.
@itsjeyd Your reasoning seems solid enough. Go ahead and keep it as is.
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.
@Kelketek OK, thanks! Will merge now.
|
@itsjeyd Left a comment/question. Otherwise, 👍 Works for me and the code makes sense. |
2c55e15 to
8af9fa2
Compare
Step Builder: Support for additional overlays for plot blocks
This PR follows up on #77 and adds functionality for defining custom overlays for plot blocks.
Testing
Add Step Builder to a unit.
Add two Mentoring Step blocks and a Review Step block to Step Builder.
Add two Rating questions to the first Mentoring Step block.
Add a Plot block to the second Mentoring Step block.
For each of the Rating questions, grab the corresponding "Question ID" from the EDIT dialog in Studio (last field, underneath "Label for high ratings").
Bring up the EDIT dialog for the Plot block and add the following to the "Claims and associated questions" field:
Make sure to replace the two hashes with the question IDs you grabbed in the previous step.
Add one or more Plot Overlay blocks to the Plot block.
For each overlay, specify plot label and point color, and add a single pair
r1, r2of coordinates to claim data, e.g.:Note that the numbers represent responses to the previously-added rating questions, so they must be between 1 and 5. (If you added multiple overlays in the previous step, you'll probably want to choose different numbers for their claim data so that corresponding points won't be stacked on top of each other when you enable the overlays for the plot in the LMS.)
Publish the unit.
Complete the first step of the unit by submitting answers to the rating questions.
Navigate to the second step. You should see a button on the right for each overlay you defined; the label of the button should correspond to the plot label you set for the overlay. The plot should not display data from overlays defined in previous steps.
Click the buttons to toggle the overlays. When you turn them on, the associated data should become visible on the plot. Point and border colors of individual overlays should match. Also, description and/or citation (if defined) should be shown on the right hand side for overlays that are currently visible. If overlays are off, the plot should not show associated points; description and citation should not be visible; border color of corresponding buttons should be reset to grey.