Learning Resource cards, list view #1054
Conversation
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 9430286 | Triggered | Generic Password | 4e121e6 | docker-compose.yml | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
jonkafton
added
Needs Review
|
@jonkafton We have some courses (most of the MITx courses) that are free but you pay for a certificate. I think we have a specific design for that . Can you pls provide a screenshot of one of those ? |
@Ferdi An example here from MITx: 
On the data on both we have Similar from Sloan: On this one we have There is a design where the price is shown as $$$: 
|
|
@jonkafton thanks. how about a screenshot for any of the courses in this list https://mitopen-rc.odl.mit.edu/search/?certification_type=completion&offered_by=mitx @steven-hatch i think Unrelated to Jon's PR, we have some data problems here @pdpinch @mbertrand Sloan courses / programs are never free |
@Ferdi here's a result from that search that's labelled as free, with certificate: 
Here we have |
There was a problem hiding this comment.
This is looking really nice. I left a few comments and one requested change—the current href breaks search page.
Spacing issue
@steven-hatch @jonkafton I also noticed a spacing issue on mid-size screens. That said, I'm not sure if the solution here is
- change the cards,
- change the search page
- make the medium breakpoint to a larger value
I do not think this spacing issue needs to be resolved in this PR.
Uploading Screen Recording 2024-06-13 at 4.45.14 PM.mov…
| const LearningPathListingPage: React.FC = () => { | ||
| const isMobile = !useMuiBreakpointAtLeast("md") |
There was a problem hiding this comment.
Mentioning: I have been avoiding using these. I'm not sure what the SSR situation is for useMuiBreakpoint. Where we can, I think CSS is a better solution, though I understand changing props via mobile vs desktop can't be done with CSS.
There was a problem hiding this comment.
It's not really needed here - I've replaced with css.
| ":hover:not(:disabled)": { | ||
| "&&": { | ||
| backgroundColor: "inherit", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
What's this for? It seems surprising to me. I wouldn't expect edge to affect the background color.
- With this style, some variants hover strangely with
edge="none"(fill, tertiary) - Without this...the new cards seem to look OK? and buttons seem to hover OK?
There was a problem hiding this comment.
The idea is that when edge is "none" we're on mobile, so no hover state. It does happen though that if you are on desktop and shrink your window, the hover area doesn't look good.
I'm anticipating this will be removed from here / updated in favor of the Card Action Buttons, though we don;t seem to have a variant of these for the edge-less mobile variant https://www.figma.com/design/Eux3guSenAFVvNHGi1Y9Wm/MIT-Design-System?node-id=4026-61418&m=dev
| overflow: hidden; | ||
| margin: 0; | ||
|
|
||
| @supports (-webkit-line-clamp: 2) { |
There was a problem hiding this comment.
I saw you used TruncateText elsewhere. Should we use that here, possibly adding the @supports to it?
| Children.forEach(children, (child) => { | ||
| if (!isValidElement(child)) return | ||
| if (child.type === Content) content = child.props.children | ||
| else if (child.type === Image) imageProps = child.props | ||
| else if (child.type === Info) info = child.props.children | ||
| else if (child.type === Title) title = child.props.children | ||
| else if (child.type === Footer) footer = child.props.children | ||
| else if (child.type === Actions) actions = child.props.children | ||
| }) |
There was a problem hiding this comment.
I'd not seen this pattern before...what is its purpose? Is it so that child order doesn't matter? I.e., so that
// 1
<ListCard>
<ListCard.Content>...</ListCard.Content>
<ListCard.Actions>...</ListCard.Actions>
</ListCard>
// 2
<ListCard>
<ListCard.Actions>...</ListCard.Actions>
<ListCard.Content>...</ListCard.Content>
</ListCard>
are treated the same?
There was a problem hiding this comment.
Yes, the pattern allows us to
- Provide slots that can be used without respect to the sequence
- Insert slots into a nested structure (example we might want to provide more granular slots inside the footer for e.g. start date, learning format)
- Give different treatment depending on the component, e.g. here we use image props if an image is provided - the image doesn't have children.
This RFC with open PR goes into the detail: https://github.com/nihgwu/rfcs/blob/neo/slots/text/0000-slots.md
| return null | ||
| } | ||
| return ( | ||
| <ListCard href={href || `?resource=${resource.id}`} className={className}> |
There was a problem hiding this comment.
Requested Change: This href does not behave correctly for the search page—it replaces the search query parameters rather than augmenting them.
This does work
const { search } = useLocation()
// then later
<ListCard
href={href || `${search}${search ? "&" : "?"}resource=${resource.id}`}
className={className}
>
HOEVER: It feels strange to me that ol-components is aware of what route the resource drawer is at. (Route-awareness belongs to page-components).
IMO, let's just specify the href via the parent. (Although, I think having a page-component with the button actions and href builtin would be reasonable. That's what we used to do, with LearningResourceCardTemplate and LearningResourceCard. Don't love the "template" language, though. )
There was a problem hiding this comment.
Yes, intended as a default, though I agree it should be controlled by the caller (and total miss on replacing the whole search!).
I've added a LearningResourceDrawer hook to provide the href.
ChristopherChudzicki
added
Waiting on author
and removed
Needs Review
jonkafton
added
Needs Review
What are the relevant tickets?
Closes Learning Resource Cards #4356
Closes Storybook stories for learning resource cards #384
Description (What does it do?)
Implements the Learning Resource Card "list" variants for desktop and mobile.
https://www.figma.com/proto/Eux3guSenAFVvNHGi1Y9Wm/MIT-Design-System?page-id=107%3A2479&node-id=3375-49853&viewport=-15774%2C-1090%2C0.55&t=cX0nylncBbGUadPG-1&scaling=min-zoom&starting-point-node-id=2197%3A31345
Applies them to:
/search/learningpathsAlso updates the
Cardcomponent in its link variant to reflect changes to the hover design and behavior, - https://www.figma.com/design/Eux3guSenAFVvNHGi1Y9Wm/MIT-Design-System?node-id=827-6168&m=dev:Screenshots (if appropriate):
Search Page:
Learning Paths page:
Card hover updates:
How can this be tested?
Load the Search and Learning Path pages at
/searchand/learningpathsand ensure the list items display correctly on desktop and at mobile screen widthsAdditional Context
The LearningResourceCard page-component (and LearningResourceCardTemplate) is still in use by the Learning Resource Details page. We'll need to refactor sortability across before we can remove it for this one.
Adds a button variation for now that matches the design with
edge="none",color="secondary". There are CardActions button designs that we haven't yet modelled in code.