[CFU] Refactor summary page question rendering back to HAML by rshipp · Pull Request #51410 · code-dot-org/code-dot-org

rshipp · 2023-04-19T17:12:45Z

Break the CheckForUnderstanding component out into several components, that can be placed on the page alongside the level question. This lets us keep the level question rendering inside Rails, which we need for the Multi levels.

There's a slight difference in padding between the top nav links and the first heading, because of the structural changes to the HTML, but I think that's fine.

Before	After

Links

jira ticket: TEACH-435

Testing story

Added eyes tests in #51517 to check for visual changes introduced by this PR.

rshipp · 2023-04-26T21:16:07Z

+/* stylelint-disable selector-pseudo-class-no-unknown */
+.levelTitle,
+.markdown h1,
+.markdown h2,
+:global(.markdown-container) h1,
+:global(.markdown-container) h2,
+:global(h1.no-underline),
+:global(h2.no-underline) {
+  border-bottom: unset;
+}
+/* stylelint-enable selector-pseudo-class-no-unknown */


mostly whitespace changes in this file since I removed the nesting so the styles would apply to the haml pieces again. this part here is the other change, where I'm adding some :global selectors, also to apply to the haml code. this seemed slightly cleaner than having another file dashboard/app/assets/stylesheets/summary.scss to duplicate css specifically for the haml, but feel free to push back on that.

rshipp · 2023-04-26T21:17:14Z

no new tests, just moving the existing ones around and removing the parts that were testing code that's now in the haml.

rshipp · 2023-04-26T21:17:45Z

+      .free-response
+        - title = level.get_property(:title)
+        - if title.present? && !in_level_group
+          %h1.no-underline= title
+        - long_instructions = level.get_property("long_instructions")
+        - if long_instructions
+          / Markdown will be rendered clientside by _free_response.js
+          .markdown-container{data: {markdown: long_instructions}}
+
+        - height = level.height || '80'
+        - placeholder = level.get_property(:placeholder) || I18n.t('free_response.placeholder')
+        %textarea.response{id: "level_#{level.id}", placeholder: placeholder, style: "height: #{height}px;", readonly: true}= last_attempt


this is copied and simplified from _free_response.html.haml, except for the addition of the .no-underline class.

rshipp · 2023-04-26T21:36:55Z

  // send them back to the level in Participant mode instead.
  if (viewAs === ViewType.Participant) {
    document.location.replace(currentLevel.url + document.location.search);
  }


a little strange to have this here in the responses component, but i wasn't sure where else to put something like this that isn't tied to any one visual element, but causes a side-effect on the page. i thought about putting it in _summary.js, but it needs access to redux, and that seems like a pain.

rshipp · 2023-04-26T21:45:47Z

-  textarea.freeResponse {
-    width: 100%;
-    box-sizing: border-box;
-    resize: vertical;
-  }


note this is removed entirely because it's back in the haml using application.scss styles

bethanyaconnor

Looks great -- just a couple questions/suggestions. Sorry that you had to do this :/

bethanyaconnor · 2023-04-28T17:15:22Z

+      levelType: level.type,
+      ...scriptData.reportingData,
+    });
+    // eslint-disable-next-line react-hooks/exhaustive-deps


Why disable this check?

i'm not sure actually, this was there before. i'll look up the original commit and see if i can remember why i did this. it might also just be wrong

It might be due to the eslint upgrades (slack thread). If possible, it would be great to clean this up while you're here but if it balloons for some reason, feel free to leave it

ah yeah it was. I added scriptData into the dependencies array and it seems happy now.

bethanyaconnor · 2023-04-28T21:00:57Z

+
+const setUpWrapper = (state = {}, jsData = {}) => {
+  const store = createStore(
+    combineReducers({


My preference here would be to export an UnconnectedSummaryTopLinks and not use redux in this test file. Personally, I think it's cleaner that way, but feel free to push back.

To expand: I'm suggested adding a link to SummaryTopLinks.jsx like export const UnconnectedSummaryTopLinks = SummaryTopLinks (possible I have my syntax wrong) then using that in this test. That was you don't have to mess around with creating the store and can instead pass props in directly.

oh! good idea!

re-reading this, I misunderstood it the first time. some questions, for my own understanding: is it common practice to test on the unconnected component like this? when should/shouldn't I do it? (e.g. why only SummaryTopLinks and not SummaryResponses?) Why don't we want the code inside the connect() to be hit by the tests too?

we do use the "unconnected" components in tests a lot through our repo (eg ShareDialog and corresponding test. That said, I forgot there's some logic in the connect function so that might be worth testing. Honestly, that's a good argument to me to keep the store in the test file.

bethanyaconnor · 2023-04-28T21:04:26Z

+    expect(wrapper.find(`.${styles.navLinks} a`).length).to.eq(1);
+  });
+
+  it('applies correct classes when rtl', () => {


bethanyaconnor · 2023-04-28T21:05:31Z

+#summary-content
+  #summary-top-links
+
+  .container#level-question


As a follow up, could we move this to a place where it's shared with the level itself? Or is it too different for that to make sense?

i think it's too different, but i'll look into it and see if i can make it work. it would be nice to not have the duplication

No worries, just curious

I'm not confident in being able to refactor _free_response to use a new thing without breaking it. I opened a task to refactor this later if we want; I'd rather do it in a separate pass, to break up the risk a bit.

thanks! definitely a follow up task if it's possible at all!

bethanyaconnor

up to you on the test -- I'm fine either way. Otherwise this looks good to me (pending a green Drone run of course)

rshipp force-pushed the summary-refactor branch 2 times, most recently from 4237d67 to 9f9725f Compare April 26, 2023 15:42

rshipp added 2 commits April 26, 2023 11:16

Refactor everything back to haml 🫠

dcae0a3

Fix trailing-comma lint issues

0e9990a

rshipp force-pushed the summary-refactor branch from 9f9725f to 0e9990a Compare April 26, 2023 16:29

rshipp added 3 commits April 26, 2023 14:05

Adjust styles

426019b

Fix missing variable

b0848ec

Fix extra underline

452b622

rshipp commented Apr 26, 2023

View reviewed changes

Add redirect back in

d7922b6

rshipp commented Apr 26, 2023

View reviewed changes

Remove unused css

038b028

rshipp commented Apr 26, 2023

View reviewed changes

Fix comment

49a583a

rshipp marked this pull request as ready for review April 27, 2023 14:41

rshipp requested a review from a team April 27, 2023 14:41

bethanyaconnor reviewed Apr 28, 2023

View reviewed changes

rshipp added 2 commits May 1, 2023 10:57

Implement some review feedback

d59eb56

Fix lint issue

0ff80b1

bethanyaconnor approved these changes May 1, 2023

View reviewed changes

Fix test css references

de6b582

rshipp merged commit 19a0544 into staging May 2, 2023

rshipp deleted the summary-refactor branch May 2, 2023 15:03

Uh oh!

Conversation

rshipp commented Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Links

Testing story

Uh oh!

rshipp Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rshipp Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rshipp Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bethanyaconnor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bethanyaconnor left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rshipp commented Apr 19, 2023 •

edited

Loading

rshipp Apr 26, 2023 •

edited

Loading

rshipp Apr 26, 2023 •

edited

Loading

rshipp Apr 26, 2023 •

edited

Loading