[CFU] Add initial free response summary page #50606

rshipp · 2023-03-06T20:20:40Z

Add an initial implementation of the "Check for Understanding" or "summary" view for free response levels.

When reviewing the JSX component, it may help to compare it with the free response files I copied this code from:

I'm not tied to the file/folder names I'm using here, so feel free to suggest different ones. I intentionally didn't refactor out components for free-response-specific things or reusable pieces, since we wanted to get Free Response out and in front of people first for feedback.

Outdated screenshot with more student answers

Links

spec: Summarizing Check for Understanding Levels
jira ticket: TEACH-289

Testing story

Manual testing in LTR and RTL languages.

dju90 · 2023-03-08T19:21:34Z

Very cool that we were able to add the "X/Y students submitted"! If you change the section in the dropdown, does the entire page reload or does it fetch new student submissions asynchronously?
Also, maybe this is just a weird thing with the screenshot - but is there a reason why there are 5 student submissions on the page but only 2 students in the teacher panel on the right?

rshipp · 2023-03-08T19:58:36Z

I'm just reloading the entire page, at least for now, because it's easier to pass in the responses from the HAML. If we wanted to change it to load responses via ajax, we could set up a new API endpoint for that.

And yeah, the 5/5 was a manual edit I did just for the screenshot, there are actually only 2 students in this section 😅.

dju90 · 2023-03-08T21:23:39Z

Yeah, don't worry about loading responses via ajax for now. Thanks! Looks great so far :)

So it doesn't apply to all tests in all files.

rshipp · 2023-03-09T19:18:33Z

apps/src/templates/levelSummary/CheckForUnderstanding.jsx

+        <textarea
+          className={styles.freeResponse}
+          id={`level_${data.level.id}`}
+          aria-label={i18n.yourAnswer()}


i don't love this aria-label, but I couldn't think of a way to do a visible label that made sense

Better this than nothing!

rshipp · 2023-03-09T19:20:49Z

apps/src/templates/levelSummary/CheckForUnderstanding.jsx

+  // To avoid confusion, if a teacher tries to view the summary as a student,
+  // send them back to the level in Participant mode instead.
+  if (viewAs === ViewType.Participant) {
+    const paramString = document.location.search
+      .replace(SUMMARY_PARAM, '')
+      .replace('&&', '&')
+      .replace('?&', '?');
+    document.location.replace(currentLevel.url + paramString);
+  }


this feels messy, but I wasn't sure a better way that handled all the cases (url?view=summary, url?otherParams=blah&view=summary&moreParams=blah, etc)

I'm interested in how a student would get into this case- would they have to type the url in manually? And this seems fine to me- I think in an ideal world (or maybe a later version?) we would have some sort of feedback/alert in red. Aside from that though, leaving them on the same level seems fine (instead of sending them back to the url they came from, which then might allow us to avoid writing them a new url).

If a student tries to visit the url, it just looks like this:

I figured it didn't matter, at least for the MVP, because there aren't any links to this page that a student would see.

I agree that an alert like the existing redirect warning if you try to visit an old version of a course, would be ideal.

rshipp · 2023-03-09T19:22:05Z

apps/src/templates/levelSummary/CheckForUnderstanding.jsx

+        <label>
+          {i18n.responsesForClassSection()}
+          <SectionSelector reloadOnChange={true} />
+        </label>


unfortunately, the SectionSelector doesn't have an id, so I have to wrap the label instead of using a for attribute

rshipp · 2023-03-09T19:23:08Z

dashboard/app/views/levels/_summary.html.haml

+      }
+    end
+
+  teacher_markdown = if level.respond_to?(:solution) && level.solution.present? && Policies::InlineAnswer.visible_for_script_level?(current_user, @script_level)


this respond_to? just makes it so the page doesn't crash if you try to view a non-free-response level in the summary view.

hannahbergam

Looks good! Some comments but none blocking. Additionally- is there a way we could test the case where a non-teacher tries to access this page? Thanks for getting all the A11y and translation work in on draft1.

hannahbergam · 2023-03-14T15:51:32Z

apps/src/templates/levelSummary/CheckForUnderstanding.jsx

+const FREE_RESPONSE = 'FreeResponse';
+
+const CheckForUnderstanding = ({
+  isRtl,


Glad this is included in V1:)

hannahbergam · 2023-03-14T15:54:26Z

apps/src/templates/levelSummary/CheckForUnderstanding.jsx

+  // To avoid confusion, if a teacher tries to view the summary as a student,
+  // send them back to the level in Participant mode instead.
+  if (viewAs === ViewType.Participant) {
+    const paramString = document.location.search
+      .replace(SUMMARY_PARAM, '')
+      .replace('&&', '&')
+      .replace('?&', '?');
+    document.location.replace(currentLevel.url + paramString);
+  }


I'm interested in how a student would get into this case- would they have to type the url in manually? And this seems fine to me- I think in an ideal world (or maybe a later version?) we would have some sort of feedback/alert in red. Aside from that though, leaving them on the same level seems fine (instead of sending them back to the url they came from, which then might allow us to avoid writing them a new url).

hannahbergam · 2023-03-14T15:55:08Z

apps/src/templates/levelSummary/CheckForUnderstanding.jsx

+
+  const questionMarkdown = data.level.properties.long_instructions;
+  const teacherMarkdown = data.teacher_markdown;
+  const height = data.level.height || '80';


Is 80 a min height? Or default height? Would it ever change?

I'm not sure about this one, I copied this from the haml. The textarea can be resized, and I assume that there's somewhere in the levelbuilder editor that lets curriculum writers set this property?

hannahbergam · 2023-03-14T15:55:41Z

apps/src/templates/levelSummary/CheckForUnderstanding.jsx

+        )}
+      </p>
+
+      {/* Question Title */}


Thanks for the comment labels here!

hannahbergam · 2023-03-14T15:55:55Z

apps/src/templates/levelSummary/CheckForUnderstanding.jsx

+        <textarea
+          className={styles.freeResponse}
+          id={`level_${data.level.id}`}
+          aria-label={i18n.yourAnswer()}


Better this than nothing!

hannahbergam · 2023-03-14T15:56:44Z

apps/src/templates/levelSummary/CheckForUnderstanding.jsx

+          id={`level_${data.level.id}`}
+          aria-label={i18n.yourAnswer()}
+          placeholder={
+            data.level.properties.placeholder || i18n.enterYourAnswerHere()


Would there a way to get a screen reader to read out this string instead? But then to your point- having it labeled as 'your answer' is helpful context too.

i could reuse this for the label, but I didn't think it made as much sense, since it's intended for the placeholder. this could be weird if curriculum designers write something important as a placeholder though 🙃

hannahbergam · 2023-03-14T15:57:39Z

apps/src/templates/levelSummary/CheckForUnderstanding.jsx

+};
+
+export default connect(
+  // NOTE: Some of this state data is loaded in by the teacher panel. If you


hannahbergam · 2023-03-14T15:58:29Z

apps/src/templates/levelSummary/check-for-understanding.module.scss

+    border-bottom: unset;
+  }
+
+  p,


Out of curiosity- how does this syntax work?

the comma selectors say to style "any of these": https://developer.mozilla.org/en-US/docs/Web/CSS/Selector_list

hannahbergam · 2023-03-14T15:59:37Z

apps/test/unit/templates/levelSummary/CheckForUnderstandingTest.jsx

+    expect(wrapper.find('SectionSelector option').length).to.eq(2);
+  });
+
+  it('applies correct classes when rtl', () => {


From Molly's feedback.

rshipp · 2023-03-14T18:53:28Z

dashboard/app/views/levels/_summary.html.haml

+  level ||= if @level.contained_levels.empty?
+    @level
+  else
+    @level.contained_levels.first
+  end


important fix to support contained levels

rshipp · 2023-04-04T19:57:37Z

For posterity, some notes on CFU implementation as of this writing:

Most of the code is in a new folder: apps/src/templates/levelSummary. There are also some pieces in apps/src/sites/studio/levels, dashboard/app/views/levels, and script_levels_controller.rb. The summary page route is /s/XXX/lessons/YYY/levels/ZZZ/summary. The summary page is still rendered by script_levels#show / levels/show.html.haml, the same as the level view page itself.

There is a new @responses variable available from the script_levels controller (and through it, all level views/partials). See dashboard/app/views/levels/_summary.html.haml partial for example usage.

Try not to expose the raw content of the level variable to the DOM, since it has a lot of things (like a full audit log with employee email addresses) that don't need to be exposed to users. Expose only what you need. This is the reason for the big js_data object in _summary.html.haml.

The summary view defaults to using answers from contained levels, whenever a contained level exists. The code for this is in the script_levels controller, if that ever needs to change.

Gotchas:

"Check for Understanding", "Summary", "Level Summary", "Gallery", are all words that have been used to describe the same page, with the student answer display.
"Check For Understanding" is also a level type that includes Free Response questions. Not all Free Response levels are CFU levels; some are Predict levels instead.
Note the comments about the Teacher Panel in the new React components. The Teacher Panel cannot be removed from the summary page without first loading student/section data into the component from the APIs.

Want to refactor:

Normalize to one name, e.g. rename "CheckForUnderstanding" to "LevelSummary" or something so that we can clear up some of the ambiguity.

Multi implementation notes:

Will need to add the entry point in two places in dashboard/app/views/levels/_single_multi.html.haml, one for contained and one for standalone.
Refactor the #summaryEntryPoint code out of apps/src/sites/studio/pages/levels/_free_response.js and into its own file, then include that in _free_response.html.haml and _single_multi.html.haml.
Break CheckForUnderstanding into reusable pieces and level-type-specific pieces, as part of implementing the summary view for Multi.
When adding any new level type, change the @responses code in script_levels_controller.rb to include that type, or you won't see student responses showing up in the summary view.

dju90 · 2023-04-04T23:36:37Z

"Check for Understanding", "Summary", "Level Summary", "Gallery", are all words that have been used to describe the same page, with the student answer display.

Oops, this is my bad :P

rshipp added 13 commits March 3, 2023 09:39

Add initial free response summary page

db5b4e9

Fix warnings

10e8d8d

Fix styling, move textarea inside freeresponse check

b30578f

Add viewAs=Participant redirect

5457b47

Remove unused code

072dab7

Handle RTL

d79b6a0

Refactor/restyle

ec04f17

Minor fixes

b15a5ba

Fix more styling

68d4d75

Fix title border

aacd947

fix next button

eddc437

Extract strings for i18n

c773188

Add corrected color to comment

75a38c3

rshipp requested a review from dju90 March 8, 2023 17:34

rshipp added 6 commits March 9, 2023 10:29

Add test setup

0b8f30a

Refactor test, add expects

09ce641

Add more tests

bba1e2c

Fix proptypes, remove sections

2d5d3c2

Move afterEach inside describe

b12fa63

So it doesn't apply to all tests in all files.

Update css comment

e2d521e

rshipp commented Mar 9, 2023

View reviewed changes

rshipp marked this pull request as ready for review March 9, 2023 19:34

rshipp requested a review from a team as a code owner March 9, 2023 19:34

rshipp requested a review from a team March 9, 2023 19:34

hannahbergam approved these changes Mar 14, 2023

View reviewed changes

rshipp added 4 commits March 14, 2023 12:25

Adjust heading styles

b46b2f5

From Molly's feedback.

Fix margins

cdf465a

Fix link text

f5ee9b7

Support contained levels, remove extra data from JS

9725676

rshipp commented Mar 14, 2023

View reviewed changes

Change syntax

6d4cf17

rshipp merged commit c0f4546 into staging Mar 14, 2023

rshipp deleted the free-response-summary branch March 14, 2023 21:42

rshipp mentioned this pull request Mar 22, 2023

[CFU] Add summary entry point for FreeResponse contained levels #50836

Merged

rshipp mentioned this pull request Mar 30, 2023

[CFU] Add summary entry point for FreeResponse contained levels (try 2) #51035

Merged

rshipp changed the title ~~[Check for Understanding] Add initial free response summary page~~ [CFU] Add initial free response summary page Apr 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CFU] Add initial free response summary page #50606

[CFU] Add initial free response summary page #50606

rshipp commented Mar 6, 2023 •

edited

dju90 commented Mar 8, 2023

rshipp commented Mar 8, 2023

dju90 commented Mar 8, 2023

rshipp Mar 9, 2023 •

edited

hannahbergam Mar 14, 2023

rshipp Mar 9, 2023

hannahbergam Mar 14, 2023

rshipp Mar 14, 2023

rshipp Mar 9, 2023

rshipp Mar 9, 2023

hannahbergam left a comment

hannahbergam Mar 14, 2023

hannahbergam Mar 14, 2023

hannahbergam Mar 14, 2023

rshipp Mar 14, 2023

hannahbergam Mar 14, 2023

hannahbergam Mar 14, 2023

hannahbergam Mar 14, 2023

rshipp Mar 14, 2023

hannahbergam Mar 14, 2023

hannahbergam Mar 14, 2023

rshipp Mar 14, 2023

hannahbergam Mar 14, 2023

rshipp Mar 14, 2023

rshipp commented Apr 4, 2023 •

edited

dju90 commented Apr 4, 2023 •

edited

[CFU] Add initial free response summary page #50606

[CFU] Add initial free response summary page #50606

Conversation

rshipp commented Mar 6, 2023 • edited

Links

Testing story

dju90 commented Mar 8, 2023

rshipp commented Mar 8, 2023

dju90 commented Mar 8, 2023

rshipp Mar 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hannahbergam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rshipp commented Apr 4, 2023 • edited

dju90 commented Apr 4, 2023 • edited

rshipp commented Mar 6, 2023 •

edited

rshipp Mar 9, 2023 •

edited

rshipp commented Apr 4, 2023 •

edited

dju90 commented Apr 4, 2023 •

edited