Skip to content
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

refactor(colorpickers, grid, loaders, notifications, tags tooltips): use transient props where appropriate #1968

Merged
merged 17 commits into from
Nov 7, 2024

Conversation

ze-flo
Copy link
Contributor

@ze-flo ze-flo commented Nov 1, 2024

Description

This PR updates various components in react-colorpickers, react-grid, react-notifications, react-loaders, and react-tooltips to use transient props where appropriate. These changes are necessary in preparation for the upgrade to [email protected] to ensure we avoid DOM violation errors after the transition.

Checklist

  • 👌 design updates will be Garden Designer approved (add the designer as a reviewer)
  • 🌐 demo is up-to-date (npm start)
  • ⬅️ renders as expected with reversed (RTL) direction
  • ⚫ renders as expected in dark mode
  • 🤘 renders as expected with Bedrock CSS (?bedrock)
  • 💂‍♂️ includes new unit tests. Maintain existing coverage (always >= 96%)
  • ♿ tested for WCAG 2.1 AA accessibility compliance
  • 📝 tested in Chrome, Firefox, Safari, and Edge

@coveralls
Copy link

coveralls commented Nov 1, 2024

Coverage Status

coverage: 95.657% (-0.2%) from 95.9%
when pulling 6547f68 on ze-flo/transient-props-rest
into bcbb17c on main.

Comment on lines 66 to 69
fontSize={size!}
color={color!}
width="80"
height="72"
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we want these props leaking to the DOM. Also, might need to git-blame, but why do we have these huge width/height values?

packages/loaders/src/elements/Inline.tsx Outdated Show resolved Hide resolved
packages/loaders/src/styled/StyledSVG.ts Outdated Show resolved Hide resolved
@@ -61,18 +61,18 @@ export const Dots = forwardRef<SVGSVGElement, IDotsProps>(

return (
<StyledSVG
$dataGardenId={COMPONENT_ID}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$dataGardenId={COMPONENT_ID}
dataGardenId={COMPONENT_ID}

The data-garden-id needs to remain intact in the DOM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dataGardenId would be filtered out by @emotion/is-prop-valid used under the hood in styled-components@v5. I've replaced it with data-garden-id.

Screenshot 2024-11-06 at 8 03 16 AM

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, bit of a bummer about that filtering.

@@ -9,13 +9,13 @@ import styled, { css, DefaultTheme, ThemeProps } from 'styled-components';
import { getColor, retrieveComponentStyles } from '@zendeskgarden/react-theming';

interface IStyledSVGProps {
dataGardenId: string;
$dataGardenId: string;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$dataGardenId: string;
dataGardenId: string;

@ze-flo ze-flo changed the title refactor(colorpickers, grid, loaders, tooltips): use transient props where appropriate refactor(colorpickers, grid, loaders, notifications, tags tooltips): use transient props where appropriate Nov 5, 2024
Copy link
Member

@jzempel jzempel left a comment

Choose a reason for hiding this comment

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

I realize this review might be premature, given the PR is still draft. But, hopefully, it can be helpful as the update progresses.

@@ -13,22 +13,26 @@ const COMPONENT_ID = 'dropdowns.multiselect_input';

interface IStyledMultiselectInputProps {
isCompact: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

Should this be $isCompact?

Copy link
Contributor Author

@ze-flo ze-flo Nov 7, 2024

Choose a reason for hiding this comment

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

Not here. This case is tricky to understand. Using $isCompact led to a snapshot mismatches.

TL&DR;

StyledMultiselectInput extends StyledInput, which in turns extends Input, a React FC that returns a StyledTextInput. Input only accepts isCompact, which is received by StyledTextInput after being renamed as transient prop $isCompact.

packages/loaders/src/elements/Dots.tsx Outdated Show resolved Hide resolved
packages/loaders/src/elements/Spinner.tsx Outdated Show resolved Hide resolved
packages/loaders/src/styled/StyledSVG.ts Outdated Show resolved Hide resolved
packages/tags/demo/stories/TagStory.spec.tsx Outdated Show resolved Hide resolved
@@ -61,18 +61,18 @@ export const Dots = forwardRef<SVGSVGElement, IDotsProps>(

return (
<StyledSVG
$dataGardenId={COMPONENT_ID}
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, bit of a bummer about that filtering.

@ze-flo ze-flo marked this pull request as ready for review November 7, 2024 21:16
@ze-flo ze-flo requested a review from a team as a code owner November 7, 2024 21:16
@ze-flo ze-flo merged commit f097e35 into main Nov 7, 2024
8 checks passed
@ze-flo ze-flo deleted the ze-flo/transient-props-rest branch November 7, 2024 21:17
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.

3 participants