ibm-products icon indicating copy to clipboard operation
ibm-products copied to clipboard

Full-page error release review

Open elycheea opened this issue 11 months ago • 1 comments

Review for release

Readiness

  • [x] One or more scenarios for a design pattern have been identified as a useful unit of functionality to publish.
  • [x] A functioning component or components delivering those scenarios have been delivered and merged to main.
  • [x] The component or components have completed an accessibility review. #4435
  • [x] One or more design maintainers have approved the implementation for those scenarios, preferably via design review. #4434

Engineering review

  • [x] Breaking changes have only been introduced with prior approval, discussion and are documented in release notes. Deprecation notices and/or feature flags are included in the prior major version.
  • [x] The implementation takes into account, and does not impair, remaining and anticipated design scenarios.
  • [x] 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.
  • [x] The UI produced is
    • [x] responsive,
    • [x] translatable[^1],
    • [x] cross-browser,
    • [x] and responds to the currently set Carbon theme.
  • [x] Components are functional components using hooks.
  • [x] Public components which produce DOM structures support className.
  • [x] Public components support a ref (via React.forwardRef).
  • [x] Public component supports a Devtools attribute
  • [ ] All significant DOM elements have meaningful classes[^2] and include the package or Carbon prefix (no hardcoded prefixes).
  • [ ] 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.
  • [x] No warnings, errors or log messages in the console.
  • [x] 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.
  • [x] Each public component SCSS lists all of the Carbon and IBM Products components imported and used by the JavaScript code and explicitly imports the SCSS for each of these components.

Standards

  • [x] No linter warnings or errors are produced.
  • [x] Carbon tokens (theme, layout, motion) are used unless the design specifies otherwise.
  • [x] All components utilizing motion must include reduced-motion queries for accessibility purposes.
  • [x] Code is formatted according to prettier rules (no use of //prettier-ignore).
  • [x] 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

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

Documentation

  • [x] Source code is satisfactorily commented and provides DocGen comments for all public components and their props.
  • [x] 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 story along with the props table.
  • [ ] There is an example (or multiple sandboxes if appropriate) on CodeSandbox and Stackblitz for the components.

[^1]: Any labels, text, or other strings within a component should use a prop. [^2]: See Carbon’s Developer Handbook for guidance on class names.

elycheea avatar Feb 26 '24 21:02 elycheea

This component looks great overall! just a few comments

FullPageError.js

  • most of the boilerplate comments should be removed
  • i think errorLabel feels slightly out of place next to title and description. i think we can just leave it as label
  • errorData contains several hardcoded strings. while the label and error kind do have a relationship and it might make sense to couple them together this is still a problem when it comes to internationalization. by default we should require the user provide title and description and let the errorData object just contain the illustrations
  • the svg's in errorData are using cx in the className field but it's not necessary
  • similar to the earlier comment about hardcoding: on line 135 there's a similar hardcoded string thing going on ↳ {kind === 'custom' ? errorLabel : 'Error ${kind}'} i think the the ↳ character is fine, but we may want to double check that for internationalization purposes, but Error ${kind} should just be replaced by label
  • if children are provided they should wrapped in our own container div. <div className={blockclass__body}>{children}</div> or something
  • since the illustrations are for decorative purposes only they should be given aria-hidden="true"

other than these comments everything else looks good. tests and coverage are covered. storybook looks good.

davidmenendez avatar Apr 02 '24 18:04 davidmenendez