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

[BPK-2269] Add theme attributes render prop component #71

Merged
merged 1 commit into from Mar 7, 2019

Conversation

k0nserv
Copy link
Contributor

@k0nserv k0nserv commented Mar 6, 2019

Copy link
Contributor

@shaundon shaundon left a comment

Choose a reason for hiding this comment

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

Diff lgtm, just gonna run it locally.

UNRELEASED.md Outdated
```javascript
const Color = withPrimaryColor((props) => (
<View style={{ backgroundColor: props.primaryColor }} />
));
Copy link
Contributor

Choose a reason for hiding this comment

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

Having a code sample is a great move - we should do this more in other PRs really!

shaundon
shaundon previously approved these changes Mar 6, 2019
Copy link
Contributor

@shaundon shaundon left a comment

Choose a reason for hiding this comment

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

Works great locally - nice one 🎉

const getDisplayName = Component =>
Component.displayName || Component.name || 'Component';

export default (defaultValue: string) => <
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I understand the advantage of this approach instead of just a function to read the property. I'm not a big fan of HOCs, specially because here we can have withPrimaryColor and withTheme together, that's a bit unecessary IMO.

If we have withTheme already I think we should just use that and provide a simple function that reads from it. I believe the fact that users will have to wrap the component with withTheme is not a big deal as the function will require the theme, and that would make it clear wrapping is required.

Another point is that this approach is not very flexible, adding a secondaryColor would be a lot more complex.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree on your point about secondaryColor the reason we are going for this over just a function where the consumer can pass the theme prop from withTheme and have the function extract the primary color. The HOC is preferable over this method because:

  1. Consumer has to do less work hence there's less risk to do it incorrectly.
  2. We maintain abstraction and can change how we do theming without consumers having to care since they are not directly using withTheme

@k0nserv
Copy link
Contributor Author

k0nserv commented Mar 6, 2019

Pushed render prop version in 4c8784f

Copy link
Contributor

@tiagoengel tiagoengel left a comment

Choose a reason for hiding this comment

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

Just one comment. LGTM otherwise

@k0nserv k0nserv force-pushed the BPK-2269-add-primary-color-utility branch 2 times, most recently from c59eed3 to 99002e6 Compare March 6, 2019 16:32
@k0nserv k0nserv changed the title [BPK-2269] Add primary color HOC [BPK-2269] Add theme attributes render prop component Mar 6, 2019
Copy link
Contributor

@matthewdavidson matthewdavidson left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

@@ -4,3 +4,12 @@

- react-native-bpk-component-text:
- Added support for changing the font using a theme prop.
- react-native-bpk-theming:
- Added a new Render Prop component `BpkThemeAttributes` to support arbitrary usage of a theme's `primaryColor` attribute. If rendered outside of a `BpkThemeProvider` with a defined theme `colorBlue500` is used instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Added a new Render Prop component `BpkThemeAttributes` to support arbitrary usage of a theme's `primaryColor` attribute. If rendered outside of a `BpkThemeProvider` with a defined theme `colorBlue500` is used instead.
- Added a new [Render Prop](https://reactjs.org/docs/render-props.html) component `BpkThemeAttributes` to support arbitrary usage of a theme's `primaryColor` attribute. If rendered outside of a `BpkThemeProvider` with a defined theme `colorBlue500` is used instead.

@k0nserv k0nserv force-pushed the BPK-2269-add-primary-color-utility branch 2 times, most recently from 05c2f07 to 2938f7a Compare March 7, 2019 09:26
@k0nserv k0nserv force-pushed the BPK-2269-add-primary-color-utility branch from 2938f7a to caa4d15 Compare March 7, 2019 09:48
@k0nserv k0nserv merged commit ddc7c0b into master Mar 7, 2019
@k0nserv k0nserv deleted the BPK-2269-add-primary-color-utility branch March 7, 2019 10:57
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.

None yet

4 participants