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

feat(exoflex): Theme Styles #362

Merged
merged 25 commits into from
Feb 24, 2020
Merged

feat(exoflex): Theme Styles #362

merged 25 commits into from
Feb 24, 2020

Conversation

danielsukmana
Copy link
Contributor

@danielsukmana danielsukmana commented Feb 19, 2020

Will resolve #320

  • Accordion
  • ActivityIndicator
  • AvatarIcon
  • AvatarImage
  • AvatarText
  • Badge
  • Button
  • Calendar
  • Checkbox
  • Chip
  • Collapsible
  • DateTimePicker
  • Divider
  • DrawerItem
  • IconButton
  • Menu
  • MenuItem
  • ProgressBar
  • RadioButton
  • SegmentedControl
  • Slider
  • Switch
  • Text
  • TextInput
  • TimePicker
  • Toast

@danielsukmana danielsukmana added the wip Work in progress label Feb 19, 2020
@danielsukmana danielsukmana marked this pull request as ready for review February 21, 2020 07:10
@danielsukmana danielsukmana added ready for review and removed wip Work in progress labels Feb 21, 2020
Copy link
Member

@sstur sstur left a comment

Choose a reason for hiding this comment

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

Wow, this is really comprehensive theme configuration!

titleStyle={titleStyle}
iconStyle={iconStyle}
titleContainerStyle={[
themeStyle?.accordion?.titleContainerStyle,
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 it's better if we flatten the themeStyle first as there could be a chance that the user using a style that generated by StyleSheet.create() for the theme style.

Ref: https://facebook.github.io/react-native/docs/stylesheet.html#flatten

Copy link
Contributor Author

@danielsukmana danielsukmana Feb 21, 2020

Choose a reason for hiding this comment

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

Would you clarify whether to apply StyleSheet.flatten specifically at this part or everywhere?

Mostly these styles are used for internal Views and Texts, which are also using styles generated by StyleSheet.create. These props here are for View, Text, and AnimatedIcon (which is an Animated.View). Is StyleSheet.flatten necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

When we create a style using StyleSheet.create() and use it, it will return the ID instead of the style object:

For example:

const styles = StyleSheet.create({
  listItem: {
    flex: 1,
    fontSize: 16,
    color: 'white',
  },
  selectedListItem: {
    color: 'green',
  },
});

StyleSheet.flatten(styles.listItem);
// returns { flex: 1, fontSize: 16, color: 'white' }
// Simply styles.listItem would return its ID (number)

That's why it would be safer to flatten it first to get the style object instead of doing optional chaining over the ID which is a number.

And I think it would be ideal if we flatten the themeStyle every time we want to use it. Because there is a high chance that the user will use the generated style object from StyleSheet.create(), not the pure object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When we create a style using StyleSheet.create() and use it, it will return the ID instead of the style object

Yup, I know about this.

But, now I'm confused. Our component props have style, someStyle, and someContainerStyle which will most likely come from StyleSheet.create(), yet we simply pass them on without calling StyleSheet.flatten(). Shall we also flatten those styles or are these two different cases?

Note that I'm following the way we're passing style from props towards the component.

Copy link
Contributor Author

@danielsukmana danielsukmana Feb 24, 2020

Choose a reason for hiding this comment

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

Discussed in person and we'll create a different PR to use StyleSheet.flatten() on all styles that come from the user.

Nevermind, after further discussion, we figured out that there was a misunderstanding and there's no need to use StyleSheet.flatten().

@danielsukmana danielsukmana merged commit 2f2fe83 into kodefox:master Feb 24, 2020
@danielsukmana danielsukmana deleted the exoflex/themeStyles branch February 24, 2020 03:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

exoflex: apply default style to exoflex components
3 participants