Skip to content

Conversation

@Parsium
Copy link
Contributor

@Parsium Parsium commented Jan 8, 2025

addresses #6517

Proposed behaviour

⚠️ BREAKING CHANGES

  • Dependencies react-dnd and react-dnd-html5-backend are now ESM-only packages.
  • Temporary incompatibility with Strict Mode in React 18

Features

  • Upgrade react-dnd and react-dnd-html5-backend to version ^16.0.0
  • Introduce support for React ^18.0.0.
  • Upgrade @tanstack/react-virtual dependency to version ^3.11.2
  • Portrait - Add data-element and data-role props for testing purposes
  • Menu - let browser handle tabbing to prevent focus on non-focusable items
  • Menu - skip non-focusable submenu items when moving focus with Up, Down, Home and End keys
  • Menu - prevent showing hover styling for non-focusable menu items

Fixes

  • Menu - ensure focus state remains consistent between tabbing and Home/End key presses
  • Menu - prevent root element from being focused when containing focusable content
  • Select - debounce scroll handler on option list to reduce frequency of onListScrollBottom calls

Current behaviour

  • Carbon only supports React v17

Checklist

  • Commits follow our style guide
  • Related issues linked in commit messages if required
  • Screenshots are included in the PR if useful
  • All themes are supported if required
  • Unit tests added or updated if required
  • Playwright automation tests added or updated if required
  • Storybook added or updated if required
  • Translations added or updated (including creating or amending translation keys table in storybook) if required
  • Typescript d.ts file added or updated if required
  • Related docs have been updated if required

QA

  • Tested in provided StackBlitz sandbox/Storybook
  • Add new Playwright test coverage if required
  • Carbon implementation matches Design System/designs
  • UI Tests GitHub check reviewed if required

Additional context

Testing instructions

Using the Storybook build, verify that the following remain unchanged for all Carbon components:

  • Visuals (appearance, layout, spacing, animations, etc.)
  • Behaviour (following interactions such as with the mouse or keyboard, screen reader behaviour)
  • Props (the set of available props and their default values)

Compare these attributes against the latest production build of Storybook, using the latest versions of Chrome, Safari and Firefox.

Exceptions

The following stories have been updated in the PR and are expected to show changes. These should not be flagged for regressions:

  • Dialog: "Editable" story (removed)
  • RadioButton: "With Legends and Labels" story (small change to component usage)
  • Menu: "Menu Full Screen with Max Width" (changes to number of menu items)

@Parsium Parsium self-assigned this Jan 8, 2025
@Parsium Parsium force-pushed the react-18-pass-2 branch 4 times, most recently from 19621c9 to 7d2f7d1 Compare January 10, 2025 09:30
@Parsium Parsium marked this pull request as ready for review January 10, 2025 11:04
@Parsium Parsium requested review from a team as code owners January 10, 2025 11:04
@Parsium Parsium marked this pull request as draft January 10, 2025 11:06
@Parsium Parsium force-pushed the react-18-pass-2 branch 6 times, most recently from eaa07b1 to 26d2ea3 Compare January 17, 2025 15:53
@Parsium Parsium changed the title feat: introduce support for react v18 feat: introduce support for react v18 (FE-6015) Jan 17, 2025
}
};
document.addEventListener("mousedown", handleClick);
const handleClickAway = () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: glad to see this has been resolved without the cludge being necessary anymore!

HTMLAnchorElement & HTMLButtonElement & HTMLDivElement,
SubmenuProps
>(
const Submenu = React.forwardRef<HTMLAnchorElement, SubmenuProps>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: is this type correct? As we can still pass an onClick to the parent item and that will cause it to use polymorphism to render a button

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it doesn't matter to TS at compile time though

Copy link
Contributor Author

@Parsium Parsium Jan 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to typing in styled-components, StyledMenuItemWrapper is inferred as a anchor element by default. Setting the ref type to HTMLAnchorElement | HTMLButtonElement results in type errors. It seems TypeScript cannot infer the ref could be of a different element type when the polymorphic as prop is passed. There may be technical detail I'm overlooking as to why its being inferred like this.

I also tried the following:

// menu-item.style.ts
const StyledMenuItemWrapper = styled(Link)<StyledMenuItemWrapperProps>`
  /* css */
`;

but this caused further type errors due incorrect props being passed to Link. So I've opted for this as a workaround until we revisit our abstractions around Link.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, it makes sense thanks for clearing it up

@quinnturner
Copy link
Contributor

I got on a call with @Parsium. Portals aren't opening when using <React.StrictMode>. I am not sure if this is considered a blocker for merging, but it's something to be aware of. If this is merged without fixing portals for StrictMode, consider adding documentation outlining this current limitation.

@quinnturner
Copy link
Contributor

quinnturner commented Jan 20, 2025

Also, as a side note: other than portals + StrictMode, our application is fully functional with both React 18 and React 19 using this pull request. It might be worth considering opening up the dependencies/peerDependencies restrictions to allow React 19.

@Parsium Parsium force-pushed the react-18-pass-2 branch 2 times, most recently from 38b63ea to 87166ec Compare February 21, 2025 12:12
in preparation for consuming latest version of React Testing Library
and its sibling packages
BREAKING CHANGE: Both packages are now ESM-only, which may affect tools like Jest
that run your project code in a Node.js environment. Adjust your bundler configuration
to handle ESM code if issues arise.
…ble items

A `MenuItem` is now only focusable when tabbing if it has an `href`, `onClick`, or `submenu`
prop, or contains a focusable child.
BREAKING CHANGE: Temporary incompatibility with React Strict Mode. In React v18, Strict Mode
now intentionally re-renders components and re-executes their effects and callbacks an extra time.
This leads to unintended side effects in several Carbon components.

A fix will be provided in advance of introducing React v19 support.

If you encounter issues, you can either disable Strict Mode globally, or apply it selectively to
specific parts of your application.
…p, Down, Home and End keys

A `MenuItem` is now only focusable via Up, Down, Home and End key navigation if it has an `href`,
`onClick`, or `submenu` prop, or contains a focusable child.
@Parsium Parsium merged commit dbe234c into master Feb 24, 2025
28 checks passed
@Parsium Parsium deleted the react-18-pass-2 branch February 24, 2025 08:55
@carbonci
Copy link
Collaborator

🎉 This PR is included in version 148.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

7 participants