Skip to content

Fix prop pass-thru for objects, including style prop #121

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 4 commits into from
Nov 10, 2022
Merged

Fix prop pass-thru for objects, including style prop #121

merged 4 commits into from
Nov 10, 2022

Conversation

brandongregoryscott
Copy link
Contributor

Resolves #120

When support was added for enhancing/serializing nested prop objects (i.e. thru selectors), we didn't account for the style prop that was previously dropping into the if (!enhancer) { preservedProps[property] = value } block, forwarding its original object value.

}

// Do not attempt to serialize style prop into a classname - it will be preserved as a native prop below
if (value && typeof value === 'object' && property !== 'style') {
Copy link
Contributor

Choose a reason for hiding this comment

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

i think im missing what this does

Copy link
Contributor Author

@brandongregoryscott brandongregoryscott Nov 7, 2022

Choose a reason for hiding this comment

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

Ah! So this block is what recursively expands/enhances props for objects, really any object under selectors:

const FancyBox = ({ children, ...rest }) => (
  <Box
    style={{ objectFit: "cover" }}
    selectors={{
      "&:hover": {
        background: "red",
      },
      "&:focus": { boxShadow: "rgba(67, 90, 111, 0.3)" },
    }}
    {...rest}
  >
    {children}
  </Box>
);

The keys that are under selectors are objects that get serialized with their selectors appended and injected into the stylesheet. However, style is an object that we shouldn't be trying to process in enhanceProps - it's a native prop that we should just pass through for consumers to use if needed (i.e., for unsupported props like objectFit https://github.com/segmentio/evergreen/blob/master/src/avatar/src/Avatar.js#L101)

Previously, we were more or less ignoring any props that didn't have a propEnhancer and forwarding it through - including those that might have object values. After writing this out, there might be other native HTML attributes that accept objects that might be clobbered by this. I'm gonna do some TypeScript-fiddling to see if I can find a list of other props that accept an object.

Edit: Actually, I think this might need some more work. If we're thinking about a polymorphic Box that can take any prop which might be a complex object, we certainly don't want to discard it, and we can't know the prop names to skip processing for ahead of time.

  interface Details {
    name: string
    description: string
  }

  interface HeaderProps {
    details: Details
  }

  const Header: React.FC<HeaderProps> = ({ details }) => (
    <React.Fragment>
      <Box>{details.name}</Box>
      <Box>{details.description}</Box>
    </React.Fragment>
  )

  const FancyHeader: React.FC<HeaderProps> = ({ details }) => (
    <Box is={Header} details={details} backgroundColor="red" />
  )

details would not be passed to Header

@brandongregoryscott brandongregoryscott marked this pull request as draft November 7, 2022 16:40
@brandongregoryscott brandongregoryscott marked this pull request as ready for review November 7, 2022 21:17
@brandongregoryscott
Copy link
Contributor Author

@dlasky I think this should be ready to re-review with the noted edge cases handled (style prop + arbitrary object props that should be forwarded)

@brandongregoryscott brandongregoryscott changed the title Add special case for 'style' prop, accompanying test Fix prop pass-thru for objects, including style prop Nov 7, 2022
@brandongregoryscott brandongregoryscott merged commit bca76ba into segmentio:master Nov 10, 2022
@brandongregoryscott brandongregoryscott deleted the fix/style-prop branch November 10, 2022 16:28
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

style prop is no longer respected
2 participants