Feat/accessibility-videos-2008: added new videos section #2009
Conversation
Co-authored-by: Taylor Seamans <[email protected]>
There was a problem hiding this comment.
Implementation looks sound. My comments regard trying to think of this work not just in solving for the immediate problem, but also how this work might be used in the future. This is a balancing act. We don't want to over optimize, but we do want to think modularly which allows for code to be composed and reused.
| @@ -21,7 +21,11 @@ export const LinkCard = ({ title, link }) => { | |||
| background="background-back" | |||
| flex={false} | |||
| > | |||
| <LinkIcon size="large" /> | |||
| {type === 'guides' ? ( | |||
There was a problem hiding this comment.
Though this works, from a design perspective, this probably doesn't scale very well. If we were to add a handful more "types", this could get messy.
What if LinkCard could be "dumb" and not have any logic in it? We could make it so that it just a presentation/layout component and the caller could pass it anything they want.
export const LinkCard = ({ icon, link, title }) => {
|
|
||
| const SectionCards = ({ items, type }) => { | ||
| const size = useContext(ResponsiveContext); | ||
| const href = |
There was a problem hiding this comment.
Similar to my previous comment, how can we make this be more "dumb" and let the caller supply the href?
What happens when we like this pattern and want to use this section of cards for a bunch of Figma tutorial videos also? We don't want to have to manage the data and mapping logic within SectionCards. Instead, let's make SectionCards be "super accommodating" (liking that phrase better than 'dumb"), so much so as "Just give me: 1) a list of objects -- I don't care what they are or represent -- they just need to have a 'title', 'link/url', [and maybe an 'icon'], and 2) an object for "see all" which has a call-to-action label and an url.
This file then could be moved under 'src/components/cards'... then the data plus the
export const GuideSection = () => <SectionCards type="guides" items={guides} />;
export const VideoSection = () => <SectionCards type="videos" items={videos} />;
could be small files living under accessibility, while the rest of the site could consume this pattern as well.
Co-authored-by: Matthew Glissmann <[email protected]>
What does this PR do?
Add new videos section to Accessibility page
Where should the reviewer start?
Accessibility page. Then LinkCards.js and SectionCards.js
What testing has been done on this PR?
Manual testing
In addition to the feature you are implementing, have you checked the following:
How should this be manually tested?
Deploy link or running locally
Any background context you want to provide?
What are the relevant issues?
#2008 #1816
Screenshots (if appropriate)
Should this PR be mentioned in Design System updates?
Yes
Is this change backwards compatible or is it a breaking change?