-
Notifications
You must be signed in to change notification settings - Fork 137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Condition builder initial release review #6032
Comments
Accessibility violations
Screen.Recording.2024-09-18.at.1.42.45.PM.mov
Screen.Recording.2024-09-18.at.2.54.05.PM.mov
Screen.Recording.2024-09-18.at.3.40.12.PM.mov
|
there's a lot to go through here, so i'm going to take my time, but i'd like to point out one issue now that's very glaring to me. in light of our recent discussions to give the user more control i think one of the best places to start is here with in
this is a good example of internal state we should avoid and let the user handle. i would simply remove this code and add a prop for the user to control the the more effects we can remove the more unintentional side effects we can mitigate. |
posting this comment from Linda for documentation purposes while we are going through the release review process as of 9/19/24
|
the other major thing that i wanted to point out is that the translations need to be handled at the prop level and not be imported directly from the component folder if we want to expose some kind of hook at the consumer level that's fine, but honestly it's probably just easier to provide the json structure in the docs and let the user pass the data structure directly. |
Hey @davidmenendez , |
Closing initial review, we'll open follow up issues for items discussed here. |
Hey, |
Review for release
Readiness
useful unit of functionality to publish.
delivered and merged to
main
.accessibility review.
scenarios, preferably via
design review.
Engineering review
and are documented in release notes. Deprecation notices and/or feature
flags are included in the prior major version.
anticipated design scenarios.
Carbon-defined conventions and are consistent internally. Where there
isn’t an existing convention to apply, ensure robust precedents are being
established.
className
.React.forwardRef
).package or Carbon prefix (no hardcoded prefixes).
title
,aria-*
, etc) are passed through to an appropriate DOM node of thecomponent as HTML attributes.
/src/components/index.js
, eachpublic component SCSS is included in
/src/components/_index.scss
, andeach public component has a flag in
package-settings.js
.components imported and used by the JavaScript code and explicitly imports
the SCSS for each of these components.
Standards
otherwise.
accessibility purposes.
//prettier-ignore
).code guidelines.
appropriate years.
Testing
outputs.
where practical.
istanbul ignore
isappropriate and not extensive.
Documentation
all public components and their props.
props and actions, and additional usage documentation and code samples are
available on the Docs story along with the props table.
and Stackblitz for the components.
Footnotes
Any labels, text, or other strings within a component should use a prop. ↩
See Carbon’s
Developer Handbook
for guidance on
class names. ↩
The text was updated successfully, but these errors were encountered: