learning resource drawer v2 run comparison table #1782

gumaerc · 2024-10-31T21:44:07Z

What are the relevant tickets?

Closes https://github.com/mitodl/hq/issues/5681#issuecomment-2450078182

Description (What does it do?)

This PR adds a table to the new learning resource drawer comparing the price and format between runs of a course, if they are different.

Screenshots (if appropriate):

How can this be tested?

If you have Posthog set up locally, enable the lr_drawer_v2 flag
If you don't have Posthog set up locally, you can force drawerV2 to be true in LearningResourceDrawer.tsx
Spin up this branch of mit-learn
Ensure you have sufficient data backpopulated into your local instance, especially Sloan courses
Search for courses that have differing run data:
- Leading the AI-Driven Organization
- Becoming a More Digitally Savvy Board Member
- Managing Product Platforms: Delivering Variety and Realizing Synergies
- Communication and Persuasion in the Digital Age
Ensure that the differing runs table shows up on each of these courses and that the style matches the design
Click some other courses on the search page and ensure that they do not show the differing runs table

jonkafton

Looks good. Veery small changes are to remove decimals from exact dollar amounts and a new line for the location display.

frontends/ol-components/src/components/LearningResourceExpanded/InfoSectionV2.tsx

jonkafton · 2024-11-07T16:01:56Z

frontends/ol-components/src/components/LearningResourceExpanded/InfoSectionV2.tsx

+    distinctLocations.length > 1
+  ) {
+    return (
+      <DifferingRuns data-testid="differing-runs-table">


Would this work better as an html table? Thinking in terms of Voiceover / accessibility.

I just looked into this a little bit. My impression is that real HTML tables are nice because screenreaders read row and column headers pretty logically and support arrow key navigation more naturally. (Example: with VoiceOver, VO = voiceover modifier combo... VO + down = navigate down, and VO reads header for new row but not for column, since it's the same).

But with the current layout of this table, it's hard to actually implement as a real HTML table:

We can make it look right by using colspan, but then "Location" loses its association with its row, which defeats the a lot of the purpose of using a real table.

I could be wrong, but I don't see a good way to make this a real table.

Also, the current navigation, at least in voiceover, seems pretty fine to me.

VO reads
"November 11, 1250 dollars, in person, location: Cambridge, MA, February 10, ..."

which seems ok.

frontends/ol-components/src/components/LearningResourceExpanded/InfoSectionV2.tsx

jonkafton · 2024-11-07T16:18:24Z

It's from a previous PR, but is the title section intended to be fixed/sticky? @steven-hatch @mbilalmughal

Thought it looked a bit odd - could just be me and not an issue on touchscreen.

gumaerc · 2024-11-07T19:00:48Z

It's from a previous PR, but is the title section intended to be fixed/sticky? @steven-hatch @mbilalmughal

Thought it looked a bit odd - could just be me and not an issue on touchscreen.

The header is meant to be sticky, but what seems to be happening here is that the image is having z-index precedence over it.

gumaerc · 2024-11-07T19:36:01Z

@jonkafton Thanks for your review. This is ready for another look. @ChristopherChudzicki and I chatted about this some more and we think it's best to not change this to an actual table element, at least not for now. Given how screen readers seem to respond to colspan, I think we should wait for an accessibility review. If they recommend that we change it, we can take another look at the design with @steven-hatch and @mbilalmughal.

jonkafton · 2024-11-08T09:08:09Z

frontends/ol-components/src/components/LearningResourceExpanded/DifferingRunsTable.tsx

+                <span>{run.delivery?.map((dm) => dm?.name).join(", ")}</span>
+              </DifferingRunData>
+            )}
+            {run.delivery.filter((d) => d.code === "in_person").length > 0 &&


Is run.delivery always an array here as there are conditionals on it above?

Yea, according to the API spec it should always be an array. I've also seen it return an array with a single element:

delivery: Array<CourseResourceDeliveryInner>

jonkafton · 2024-11-08T09:09:18Z

frontends/ol-components/src/components/LearningResourceExpanded/LearningResourceExpandedV2.tsx

@@ -88,6 +88,7 @@ const Image = styled(NextImage)({
  borderRadius: "8px",
  width: "100%",
  objectFit: "cover",
+  zIndex: -1,


jonkafton

Looking good. Approving with potential change needed for optional chaining on https://github.com/mitodl/mit-learn/pull/1782/files#diff-612b73fc63762015de6077e1ac1b79ec8489f6adf644241585eba9bcfe39adc3R123

jonkafton · 2024-11-08T09:13:50Z

frontends/ol-utilities/src/learning-resources/pricing.ts

+    .sort(
+      (a: LearningResourcePrice, b: LearningResourcePrice) =>
+        Number(a.amount) - Number(b.amount),
+    )


* add first draft of run comparison table * hide labels on mobile * update differing run table styles * add tests * display table if location is different * move DifferingRunsTable to its own file * use getDisplayPrice * fix CTA image z-index issue * remove errant console.log * fix location display * remove duplicate sort

gumaerc added the Work in Progress label Oct 31, 2024

gumaerc force-pushed the cg/lrd-v2-run-comparison branch 2 times, most recently from 3c8e756 to 2c9c5da Compare November 5, 2024 22:03

gumaerc marked this pull request as ready for review November 6, 2024 18:37

gumaerc added Needs Review An open Pull Request that is ready for review and removed Work in Progress labels Nov 6, 2024

jonkafton requested changes Nov 7, 2024

View reviewed changes

gumaerc force-pushed the cg/lrd-v2-run-comparison branch from 4d35704 to 2723143 Compare November 7, 2024 20:05

jonkafton reviewed Nov 8, 2024

View reviewed changes

jonkafton approved these changes Nov 8, 2024

View reviewed changes

jonkafton reviewed Nov 8, 2024

View reviewed changes

gumaerc added 10 commits November 8, 2024 12:04

add first draft of run comparison table

83e547e

hide labels on mobile

2d74e19

update differing run table styles

e1772a4

add tests

cf5578d

display table if location is different

b1ed214

move DifferingRunsTable to its own file

21171ac

use getDisplayPrice

b084be9

fix CTA image z-index issue

e8565da

remove errant console.log

b14f371

fix location display

7f0e85f

gumaerc force-pushed the cg/lrd-v2-run-comparison branch from 2723143 to 7f0e85f Compare November 8, 2024 17:05

remove duplicate sort

205ad11

gumaerc merged commit f27d2ce into main Nov 8, 2024
11 checks passed

This was referenced Nov 14, 2024

Release 0.25.0 #1824

Closed

Release 0.25.0 #1832

Merged

rhysyngsun deleted the cg/lrd-v2-run-comparison branch February 7, 2025 20:42

learning resource drawer v2 run comparison table #1782

learning resource drawer v2 run comparison table #1782

Uh oh!

Conversation

gumaerc commented Oct 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What are the relevant tickets?

Description (What does it do?)

Screenshots (if appropriate):

How can this be tested?

Uh oh!

jonkafton left a comment

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

jonkafton commented Nov 7, 2024

Uh oh!

gumaerc commented Nov 7, 2024

Uh oh!

gumaerc commented Nov 7, 2024

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!

jonkafton 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!

Uh oh!

Uh oh!

gumaerc commented Oct 31, 2024 •

edited

Loading