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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement shouldForwardProp for React Native/Primitives

Merged

Conversation

macintoshhelper
Copy link
Contributor

@macintoshhelper macintoshhelper commented Apr 11, 2020

Tested with react-figma (with styled-components/primitives). Should work with React Native, but would be great if someone could test it before merging 馃檪.

I had to do npm i -D bundlesize jest in the package folder. Should I commit the package.json with them as local dependencies? I try to avoid global dependencies where possible (to avoid version mismatches between packages, and make sure people have up-to-date versions).

fixes #3093

@macintoshhelper
Copy link
Contributor Author

macintoshhelper commented Apr 11, 2020

I copied this test over from the web entrypoint tests, but it fails, not sure if it's because of the use of findByProps, or test renderer doesn't work for children in React Native? Bit strange, should be fine to leave it out though

    it('should inherit shouldForwardProp for wrapped styled components', () => {
      const View1 = styled.View.withConfig({
        shouldForwardProp: prop => prop !== 'color'
      })`
        background-color: ${({ color }) => color};
      `;
      const View2 = styled(View1)``;
      const wrapper = TestRenderer.create(
        <Fragment>
          <View1 color="red" id="test-1" />
          <View2 color="green" id="test-2" />
        </Fragment>
      );
      const view1 = wrapper.root.findByProps({ id: 'test-1' });
      const view2 = wrapper.root.findByProps({ id: 'test-2' });

      console.dir(view1.props);
      expect(view1.props.style).toEqual([{ color: 'red' }]);
      expect(view2.props.style).toEqual([{ color: 'green' }]);
      expect(wrapper.toJSON()).toMatchSnapshot();
    });

@probablyup
Copy link
Contributor

probablyup commented Apr 13, 2020

@macintoshhelper if you use yarn it should all work as expected because we use yarn "workspaces"

@@ -44,15 +45,20 @@ class StyledNativeComponent extends Component<*, *> {
this.props
);

const isTargetTag = isTag(elementToBeRendered);
const computedProps = this.attrs !== props ? { ...props, ...this.attrs } : props;
const propFilterFn = shouldForwardProp || (isTargetTag && validAttr);
Copy link
Contributor

@probablyup probablyup Apr 13, 2020

Choose a reason for hiding this comment

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

So for native since there's no "default" prop whitelist to speak of, the default validator fn can just be a function that returns true.

Copy link
Contributor Author

@macintoshhelper macintoshhelper Apr 13, 2020

Choose a reason for hiding this comment

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

Does a3569b2 address this? Updated the comments slightly too.

CHANGELOG.md Outdated
### Bugfixes

- Added `useTheme` hook to named exports for react-primitives entrypoint (see [#2982](https://github.com/styled-components/styled-components/pull/2982)) thanks @jladuval!
- Added `useTheme` hook to named exports for react-primitives entrypoint (see [#2982](https://github.com/styled-components/styled-components/pull/3108))
Copy link
Contributor

@probablyup probablyup Apr 24, 2020

Choose a reason for hiding this comment

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

I think this might have been a mistake?

Copy link
Contributor Author

@macintoshhelper macintoshhelper Apr 24, 2020

Choose a reason for hiding this comment

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

There were two PRs for the useTheme export, a previous one by jladuval exporting useTheme from the native entrypoint, but I created one recently exporting useTheme in the primitives entrypoint, but had referenced the original native PR in the changelog.

Someone added a thanks note in the CHANGELOG.md which is in master now, referencing the author of the native PR, because I had linked to the wrong PR. Was correcting the PR reference here to the one that I opened for primitives, since the native one has already been published and is already in the CHANGELOG.md for an earlier release.

I hope that makes sense 馃槃.

Copy link
Contributor

@probablyup probablyup Apr 24, 2020

Choose a reason for hiding this comment

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

Oh ok, can you fix the link text to match the PR that was linked? (it says 2982 instead of 3108)

Copy link
Contributor Author

@macintoshhelper macintoshhelper Apr 24, 2020

Choose a reason for hiding this comment

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

2982 has already been released and is in the CHANGELOG.md already, it should've been linked to here: https://github.com/styled-components/styled-components/blob/88ec191fdef0e0a07b40190170d60b01dee261a1/CHANGELOG.md#v501---2020-02-04

3108 is the correct PR for adding useTheme to the react-primitives entrypoint.

I had linked to 2982 originally here: https://github.com/styled-components/styled-components/pull/3108/files#diff-4ac32a78649ca5bdd8e0ba38b7006a1eR9 鈥 but should be 3108 I think?

Copy link
Contributor Author

@macintoshhelper macintoshhelper Apr 24, 2020

Choose a reason for hiding this comment

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

Are you asking me to revert it back to Added ``useTheme`` hook to named exports for react-primitives entrypoint (see [#2982](https://github.com/styled-components/styled-components/pull/2982)) thanks @jladuval!?

I can do so, but 2982 is for the native PR though. 3108 is for my react-primitives entrypoint PR, which this changelog entry is referring to.

This is the commit where the wrong thanks note (for native, not primitives) was added: 01e5747#diff-4ac32a78649ca5bdd8e0ba38b7006a1eL9

Sorry for the confusion! I'm a bit new to changelogs. Maybe worth creating a new issue/docs for changelogs to avoid confusion in the future around merge conflicts, PR references, etc?

probablyup
probablyup previously approved these changes Apr 24, 2020
Copy link
Contributor

@probablyup probablyup left a comment

LGTM other than the one changelog nit

@probablyup probablyup merged commit 4add697 into styled-components:master Apr 24, 2020
1 check was pending
@probablyup
Copy link
Contributor

probablyup commented Apr 24, 2020

Thanks for your work on this!

@macintoshhelper
Copy link
Contributor Author

macintoshhelper commented Apr 24, 2020

Np. Thanks for merging 馃檪. Should I open an issue for discussing changelog merge conflicts? Maybe worth documenting a process in CHANGELOG.md for submitting PRs first, then submitting a new commit with a link in the changelog to the newly opened PR to help people avoid confusion in the future?

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.

2 participants