Condition builder initial release review

[elycheea]

Review for release

Readiness

• [ ] 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.
• [ ] The component or components have completed an accessibility review.
• [ ] One or more design maintainers have approved the implementation for those scenarios, preferably via design review.

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.
• [ ] The UI produced is
  • [x] responsive,
  • [x] translatable[^1],
  • [ ] 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
• [x] All significant DOM elements have meaningful classes[^2] and include the package or Carbon prefix (no hardcoded prefixes).
• [x] 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.
• [ ] 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.
• [x] 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.
• [ ] The tests validate the behaviors and interactions defined in the design where practical.
• [ ] The tests achieve at least 80% coverage. Usage of istanbul ignore is appropriate and not extensive.
• [ ] 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.

[makafsal]

@amal-k-joy

Accessibility violations

• [ ] None of the descendent elements with "row" role is tabbable

• [ ] Verify the <div> element with "treegrid" role has keyboard access

• [ ] The element with role "treegrid" owns the child element with the role "grid" that is not one of the allowed role(s): "row, rowgroup"

• [ ] Flickering while hovering the close Delete Condition button

https://github.com/user-attachments/assets/2eafdc04-0ea9-4414-930c-25a1e5252424

• [ ] In Firefox, when pressing Esc to close the Date Picker, it toggles the fullscreen. (preventDefault)

https://github.com/user-attachments/assets/aa76f403-10df-40df-84a9-d10f2627dfcc

• [ ] Remove console from stories and tests files
• [ ] Remove unwanted comments

• [ ] Error in console when pressing up and down arrows in the Price field

https://github.com/user-attachments/assets/10f13524-9d4c-416a-937b-cb2dbca60517

• [ ] AVT covers only the initial state, please provide more coverage
• [ ] There is no example, please provide CodeSandbox and Stackblitz

[davidmenendez]

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 ConditionBuilder.

in ConditionBuilderContent.tsx we have this effect running to declare the builder is active

  useEffect(() => {
    if (rootState?.groups?.length) {
      setIsConditionBuilderActive(true);
    } else {
      setIsConditionBuilderActive(false);
    }

    if (getConditionState) {
      getConditionState(rootState ?? {});
    }

    // eslint-disable-next-line react-hooks/exhaustive-deps
  }, [rootState]);

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 open or active state of the component.

the more effects we can remove the more unintentional side effects we can mitigate.

[davidmenendez]

posting this comment from Linda for documentation purposes while we are going through the release review process as of 9/19/24

Recap on Condition Builder from today’s standup: Okay to merge PR for the usage docs to go live on new website Hold on bringing to stable Document only any a11y issues from either AvT or screenreader/voiceover. Issues created for bugs should be documented but don’t have to pick up work on those bugs.

[davidmenendez]

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 import { useTranslations } from '../utils/useTranslations';

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. translations={clientTranslationObj}

[amal-k-joy]

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 ConditionBuilder.

in ConditionBuilderContent.tsx we have this effect running to declare the builder is active

  useEffect(() => {
    if (rootState?.groups?.length) {
      setIsConditionBuilderActive(true);
    } else {
      setIsConditionBuilderActive(false);
    }

    if (getConditionState) {
      getConditionState(rootState ?? {});
    }

    // eslint-disable-next-line react-hooks/exhaustive-deps
  }, [rootState]);

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 open or active state of the component.

the more effects we can remove the more unintentional side effects we can mitigate.

Hey @davidmenendez , Thanks for looking into this. This useEffect is required when the user deletes all the conditions , then we need to show back the Add condition button. So we need to listen to the root state for this. Also we have kept the Add conditionbutton as part our component.

[matthewgallo]

Closing initial review, we'll open follow up issues for items discussed here.

[amal-k-joy]

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 import { useTranslations } from '../utils/useTranslations';

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. translations={clientTranslationObj}

Hey, Based on our earlier discussion on the implementation of translation, we had decided to implement by exposing a prop translateWithId, which is a callback to the user that will return the translated text. This approach aligns with core carbon as well. Here useTranslations is just a local hook that will grab the required text to our components either from users callback or from default translation object. May be we can mention the expected translations keys in the story book docs