Card release review

[davidmenendez]

Review for release

Readiness

• One or more scenarios for a design pattern have been identified as a useful unit of functionality to publish.
• A functioning component or components delivering those scenarios have been delivered and merged to master.
• Design maintainer has approved the implementation for those scenarios (via a comment on the relevant issue/epic) - aka. Design review completed

Engineering review

• Breaking changes have only been introduced with prior approval, discussion and documented in release notes. Ideally deprecation notices in the prior major version must have been added in a timely fashion.
• The implementation takes into account, and does not impair, remaining and anticipated design scenarios.
• Public component features (names, props, etc) are consistent with Carbon-defined conventions and are consistent internally. Where there isn't an existing convention to apply, ensure robust precedents are being established.
• The UI produced is accessible, responsive, translatable, cross-browser, and responds to the currently set Carbon theme.
• Components are functional components using hooks.
• Public components which produce DOM structures support className.
• Public components support a ref (via React.forwardRef).
• All significant DOM elements have meaningful classes.
• Additional attributes that are not identified as props (such as title, aria-*, etc) are passed through to an appropriate DOM node of the component as HTML attributes.
• No warnings, errors or log messages in the console.
• Each public component JS is exported in /src/components/index.js, each public component SCSS is included in /src/components/_index.scss, and each public component has a flag in package-settings.js.

Standards

• No linter warnings or errors are produced.
• Carbon tokens (theme, layout, motion) are used unless the design specifies otherwise.
• Code is formatted according to prettier rules (no use of //prettier-ignore).
• Code is clear, maintainable and follows standard React practices and the code guidelines.
• Copyright header in every source file (js, css, scss etc.) with the appropriate years.

Testing

• There is a set of test cases for the components.
• The tests exercise all inputs (props, slots, etc) and verify appropriate outputs.
• The tests validate the behaviors and interactions defined in the design where practical.
• The tests achieve 100% coverage. Usage of istanbul ignore is appropriate and not extensive.
• No warnings, errors or log messages in the test output.

Documentation

• Source code is satisfactorily commented and provides DocGen comments for all public components and their props.
• There is a story for each design scenario. The stories expose all relevant props and actions, and additional usage documentation and code samples are available on the 'Docs' tab along with the props table.
• There is a sandbox (or multiple sandboxes if appropriate) on CodeSandbox for the components.

[sydrosa]

• The prop caption seems like it'd be more suited to either description or subtitle. do you have thoughts on this? How long will the text here possibly be?
• In CardHeader.js, we noticed that he label, title and caption are all <p> tags. Can we potentially change to something more semantic? perhaps an <h6> for the title?
• The story for Clickable Cards isn't consistent with it's use since the clickableZone is defaulted to one. Can you make this defaulted to two?
• In your stories, remove the function before the action on onClick and onKeyDown:
  onClick: action('on click')
• When you add the mediaPosition to left on a smaller column size, there's some wonky behavior where the content looks quite cramped. Can we double check with design that this is the intended behavior?
• Does onKeyDown and onClick props differ? Seems like they would be the same thing and we don't want our end users to have to pass in the same function twice.
• In ExpressiveCard.js you're getting a console error -- go ahead and add mediaRatio to prepareProps to remove this
• You'll need to import ExpressiveCard and ProductiveCard in _index.scss as well as the React components in index.js
• In your first test on line 19, can you add an expect() function to ensure that test is checking for that component in the dom? expect(Card).toBeinDocument or something
• on line 35, in Card.test.js, it might be helpful to be more specific with what the test is doing. "Renders expressive card with action icons and ensures that each click is being called"
• On line 119 (and 138), in the tests, we should probably have an expectation after the reRender. If you're testing for action placement, have the expected behavior in expect() and add that to the test description.
• Let's go ahead and add the accessibility checker to your tests in Card.test.js -- you can see an example in ExampleComponent.test.js
• In your documentation (.mdx) files it looks like the code snippet for both Productive and Expressive doesn't show the actual source code... we weren't able to figure out why. Could you look into it?
• Can you add a few usage guidelines from the Cards documentation into your component docs?
• Add codesandbox

[matthewgallo]

[sydrosa]

Once you address the above, go ahead and do the following!

Thanks again, @davidmenendez 🎉

• the flags for the components in package-settings.js should be changed to true.
• the component SCSS should be included in /src/components/_index-released-only.scss.
• run the tests, recreating snapshots (using -u), and check in the updated public CSS snapshot. NB it is sufficient to run yarn jest styles -u to complete the snapshot updates.

[davidmenendez]

The prop caption seems like it'd be more suited to either description or subtitle. do you have thoughts on this? How long will the text here possibly be?

i believe i used caption originally because that was the term used in the design documentation, but i have no strong feeling towards this and i'll change it to description.

In CardHeader.js, we noticed that he label, title and caption are all

tags. Can we potentially change to something more semantic? perhaps an

for the title?

my only problem with this is that since any combination of these can be used and the design makes it so that semantically it doesn't make sense anyways because label is smaller than title but resides above it. i don't think there's anything wrong here with using p tags.

When you add the mediaPosition to left on a smaller column size, there's some wonky behavior where the content looks quite cramped. Can we double check with design that this is the intended behavior?

it's definitely working as intended. the problem is trying to use a one size fits all piece of content there. the AspectRatio component that i'm using there can only do so much when sized in these weird ways. this is a case where the story isn't flexible enough to make it work. it'll look fine in a real use case when an appropriately sized piece of content is being used.

Does onKeyDown and onClick props differ? Seems like they would be the same thing and we don't want our end users to have to pass in the same function twice.

it's entirely possible that the function is different. we shouldn't assume otherwise and always provide the consumer with maximum flexibility.

[sydrosa]

my only problem with this is that since any combination of these can be used and the design makes it so that semantically it doesn't make sense anyways because label is smaller than title but resides above it. i don't think there's anything wrong here with using p tags.

I can see your point for all of these with the exception of title. I think at a minimum, the title should be wrapped around an h6, which is the lowest level header as far as hierarchy goes and won't introduce (or shouldn't introduce) conflicts.

[davidmenendez]

In your documentation (.mdx) files it looks like the code snippet for both Productive and Expressive doesn't show the actual source code... we weren't able to figure out why. Could you look into it?

found the solution here storybookjs/storybook#12596 (comment)

[davidmenendez]

Can you add a few usage guidelines from the Cards documentation into your component docs?

honestly I don't really see a reason to do this. all i'd be doing is copying and pasting content from the designated design usage guidelines, which i'm already linking to. it doesn't seem necessary to me when all the pertinent information already exists elsewhere.