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

Pass and sanitize commonProps passed to GroupHeader and Input #4391

Merged
merged 5 commits into from
Mar 5, 2021

Conversation

ebonow
Copy link
Collaborator

@ebonow ebonow commented Jan 22, 2021

Purpose:

When styling components, it would be beneficial to have commonProps accessible as the documentation states that they should be. This also resolves a bug introduced in v3.2

Proposal:

Currently these two components do not use commonProps. It is likely because they pass extra props directly to the components wrapper as DOM attributes. It's worth noting that they do not follow the same innerProps paradigm as the other components.

Since all the cleanedProps are passed directly to the DOM as attributes, it was just a matter of removing the commonProp keys from the props as a util function.

Resolves:

Issue: "input" object of the styles object should receive the same state as the other components #3982

Issue: Unreported side effect from
Pass down extra props to group header as item.data #3046

Result of the PR is that GroupHeader now renders with data attribute in the DOM represented as data=[Object Object]
Screen Shot 2021-01-21 at 4 12 55 PM

The included code resolves this by destructuring data from the sanitized props prior to passing them in as "innerProps"

@changeset-bot
Copy link

changeset-bot bot commented Jan 22, 2021

🦋 Changeset detected

Latest commit: 665f7ae

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
react-select Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@ebonow ebonow added this to the 3.3 milestone Jan 22, 2021
@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 22, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 665f7ae:

Sandbox Source
react-codesandboxer-example Configuration

@Methuselah96
Copy link
Collaborator

Methuselah96 commented Jan 22, 2021

I'm probably missing something, but I don't see how the commonProps are getting passed to GroupHeading (besides the ones that are manually specified (i.e., selectProps, them, getStyles, and cx)).

@ebonow
Copy link
Collaborator Author

ebonow commented Feb 4, 2021

Common props weren't being passed and this aims to add them for the sake of consistency.

The previous implementation was...

  const { className, cx, getStyles, theme, selectProps, ...cleanProps } = props;
   return (
    <div
      css={getStyles('groupHeading', { theme, ...cleanProps })}
      className={cx({ 'group-heading': true }, className)}
      {...cleanProps}
    />
  );

Both GroupHeading and Input lack innerProps and instead all the extra props are passed to the wrapper DOM element as if they were innerProps. My guess is this is why there are no commonProps added.

The intention with this was to create a function that would cleanse the common props in a similar way, so that all of the commonProps are available to the styles api, but then sanitized for the DOM in a consistent way.

@Methuselah96
Copy link
Collaborator

Okay, got it, should we go ahead and pass ...commonProps to GroupHeading then?

@Methuselah96 Methuselah96 modified the milestones: 4.1, 4.2 Feb 5, 2021
@JedWatson JedWatson enabled auto-merge March 4, 2021 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"input" object of the styles object should receive the same state as the other components
3 participants