feat(get-started): new component #5054
Conversation
✅ Deploy Preview for carbon-for-ibm-products ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
elycheea
added
the
status: needs design review 🎨
ariellalgilmore
requested review from
kennylam and
elycheea
and removed request for
a team
|
|
|
|
@bndege2 just pushed updates! The cards are flexible in size, so if the user puts them in the container like you show above then they will fit in that container |
|
@ariellalgilmore updates look good! |
There was a problem hiding this comment.
Overall looks great but I have some concerns about the structure and where we’re setting props. I’m guessing there’s a few limitations because of the way the internal Card is set up, but I feel like where we can, we should keep GetStarted specific props on the GetStarted component itself.
Happy to discuss these further too since I’m sure there’s a lot more complexity to my suggestions than they appear on paper. Might also be worth pulling in @davidmenendez for his thoughts since he worked on the initial card variants. :)
packages/ibm-products-styles/src/components/GetStarted/_get-started.scss
Outdated
Show resolved
Hide resolved
| @@ -41,10 +42,14 @@ export let Card = forwardRef( | |||
| // The component props, in alphabetical order (for consistency). | |||
| actionIcons = defaults.actionIcons, | |||
| actionsPlacement = defaults.actionsPlacement, | |||
| metaData = defaults.actionIcons, | |||
There was a problem hiding this comment.
Metadata’s common enough a term that I don’t think we need to camelCase this one. 🐫
| metaData = defaults.actionIcons, | |
| metadata = defaults.actionIcons, |
There was a problem hiding this comment.
I do like the thought of reusing what we have, but also don’t think the metadata really counts as “actions” semantically? 🤔
| */ | ||
| export let GetStarted = React.forwardRef(({ ...rest }, ref) => { | ||
| return ( | ||
| <Card getStarted ref={ref} {...rest} {...getDevtoolsProps(componentName)} /> |
There was a problem hiding this comment.
I saw your comment that we used the internal Card instead of ProductiveCard — were we unable to get the actions working using ProductiveCard?
I was thinking originally something like ...
<ProductiveCard ref={ref} {...rest} {...getDevtoolsProps(componentName)}>
<div className={`${blockClass}__header`}>
{sequence && (
<div className={`${blockClass}__sequence`}>{sequence}</div>
)}
{status && (
<div className={`${blockClass}__status`}>
...
</div>
)}
</div>
<div className={`${blockClass}__footer`}>
<div className={`${blockClass}__metadata`}>
...
</div>
</div>
</ProductiveCard>
... but I’m guessing this was a bit more complicated because of the positioning of sequence/status before the title/label and other media elements. 🤔
Still, anything we can keep specific to just the GetStarted component will likely help reduce complexity at the internal Card level longer term.
packages/ibm-products/src/components/GetStarted/GetStarted.stories.jsx
Outdated
Show resolved
Hide resolved
| @@ -41,10 +42,14 @@ export let Card = forwardRef( | |||
| // The component props, in alphabetical order (for consistency). | |||
| actionIcons = defaults.actionIcons, | |||
| actionsPlacement = defaults.actionsPlacement, | |||
| metadata = defaults.actionIcons, | |||
There was a problem hiding this comment.
is there any specific reason why metadata is required and we can't just use actionIcons?
There was a problem hiding this comment.
when discussing with the PLG group, actionIcons was an icon you would take action on like in the other cards, but in this case it's more describing the card... so we thought metadata was a better prop name, but code-wise almost the same thing
|
@davidmenendez thanks for the review!! updated the getStarted component with your suggestions! I know we are still discussing card, but wanted to update here in the meantime! |
5d7208f
* feat(get-started): new component * fix(lint): update files * fix(footer): typo * fix(media): center align * fix(getStarted): mobile and step to sequence * fix(getStarted): move to jsx * fix(getStarted): picture and font * fix(getStarted): cleanup * fix(getStarted): cleanup * fix(story): update copy * feat(get-started): add avt test * fix(getStarted): update from review comments * fix(get-started): resolve comments * fix(get-started): use controls not argstable * fix(get-started): add card to name
Contributes to #4809
Adds new getStarted component
What did you change?
...still waiting on mobile updates from design
How did you test and verify your work?