Skip to content

feat: #187 add gap property #188

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

Merged
merged 1 commit into from
Jan 23, 2023
Merged

Conversation

mlecoq
Copy link
Contributor

@mlecoq mlecoq commented Jan 15, 2023

Resolves #187

I have upgraded the lib to support new gap property of RN 0.71

I had to make some adjustments on types to fix typescript issues

@mlecoq
Copy link
Contributor Author

mlecoq commented Jan 15, 2023

cla signed

@mattfrances
Copy link
Contributor

@mlecoq thanks for doing this! To isolate the changes of this PR to just be related to the gap property, I'll be making a PR upgrading this package's React Native version to 0.71. Once that's done, you'll be able to rebase and push only the changes specific to the gap property. The upgrade PR should be up within the next few days, thanks for your patience!

@mattfrances mattfrances self-requested a review January 18, 2023 16:38
@mattfrances mattfrances mentioned this pull request Jan 19, 2023
2 tasks
@mattfrances
Copy link
Contributor

@mlecoq Please rebase from main as the React Native version has been upgraded to 0.71 (#207)

@mlecoq mlecoq force-pushed the feat/addGapProperty branch from 39f47eb to fdf04f7 Compare January 19, 2023 16:51
@mlecoq
Copy link
Contributor Author

mlecoq commented Jan 19, 2023

@mattfrances done !

Copy link
Contributor

@fortmarek fortmarek left a comment

Choose a reason for hiding this comment

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

This looks good!

I know we haven't done this for the other properties since it is somewhat redundant to test that the individual props are passed to style based on the true flag in restyleFunction. But I'd still really love if we could get some basic coverage at least for new props. That is there should be some test that fails before implementing the rest of the PR.

I suggest adding a test like this to createRestyleComponent.test.tsx:

    it('uses gap values from the theme', () => {
      const {root} = render(
        <ThemeProvider theme={theme}>
          <Component gap="s" columnGap="s" rowGap="s" />
        </ThemeProvider>,
      );
      expect(root.findByType(View).props).toStrictEqual({
        style: [{gap: 8, columnGap: 8, rowGap: 8}],
      });
    });

We should also update the docs by adding the gap props here in README.

Additionally, let's also add an entry into CHANGELOG 🙏

@mlecoq
Copy link
Contributor Author

mlecoq commented Jan 20, 2023

Ok I'm on it

@mlecoq mlecoq force-pushed the feat/addGapProperty branch from fdf04f7 to 463ddae Compare January 20, 2023 12:24
@mlecoq
Copy link
Contributor Author

mlecoq commented Jan 20, 2023

@fortmarek done !

@fortmarek
Copy link
Contributor

@mlecoq could you also update the README with the docs for the gap property? Afterwards, LGTM 🚀

@mlecoq mlecoq force-pushed the feat/addGapProperty branch from 463ddae to 6e3ceb5 Compare January 20, 2023 17:53
@mlecoq
Copy link
Contributor Author

mlecoq commented Jan 20, 2023

@mlecoq could you also update the README with the docs for the gap property? Afterwards, LGTM 🚀

I have added gap properties to the list + updated an example where columnGap looks more appropriated than marginRight

@hamdij0maa
Copy link

when will be this update released please?

@mlecoq
Copy link
Contributor Author

mlecoq commented Jan 21, 2023

You can use a patch with patch-package if you need it now

You just have to add 3 lines in SpacingProperties in file src/restyleFunctions.ts

gap:true,
columnGap: true,
rowGap: true

Copy link
Contributor

@fortmarek fortmarek left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contribution ❤️ Let's ship it!
We'll do a release soon with this one 🙂

@fortmarek fortmarek merged commit 2a87618 into Shopify:master Jan 23, 2023
Comment on lines +51 to +52
rG: 'rowGap',
cG: 'columnGap',
Copy link

@matinzd matinzd Jan 28, 2023

Choose a reason for hiding this comment

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

Wasn't it better to keep the shorthand version consistent? rg and cg instead of rG and cG?

Copy link
Contributor Author

@mlecoq mlecoq Jan 28, 2023

Choose a reason for hiding this comment

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

You're right, I don't use shorthands and didn't notice they are in lowercase

@matinzd I will make a new PR to fix my mistake

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see #218

Copy link

Choose a reason for hiding this comment

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

Thanks! That would be great :)

@hamdij0maa
Copy link

@fortmarek please any idea about adding : the cornerCurve prop ? added in RN 0.71.X

@mlecoq mlecoq deleted the feat/addGapProperty branch January 28, 2023 09:35
@mlecoq
Copy link
Contributor Author

mlecoq commented Jan 28, 2023

@fortmarek please any idea about adding : the cornerCurve prop ? added in RN 0.71.X

borderCurve you mean ?

@hamdij0maa
Copy link

@mlecoq yes hhh

@mlecoq
Copy link
Contributor Author

mlecoq commented Jan 28, 2023

I'll make a PR

@mlecoq
Copy link
Contributor Author

mlecoq commented Jan 28, 2023

it's missing in react native types, have to make a PR on types repo first

@hamdij0maa
Copy link

yes, it works only on iOS also I think

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.

Flex gap support RN 0.71.X
5 participants