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(components): add theming hooks #782

Merged
merged 6 commits into from
Dec 20, 2019
Merged

feat(components): add theming hooks #782

merged 6 commits into from
Dec 20, 2019

Conversation

artyorsh
Copy link
Collaborator

@artyorsh artyorsh commented Dec 19, 2019

Please read and mark the following check list before creating a pull request:

Short description of what this resolves:

  • Adds useTheme hook:
    Returns the theme from nearest ThemeProvider
    ([feature request] useTheme hook #676, Hooks #752)

  • Adds useStyleSheet hook:
    Takes a theme provided by ApplicationProvider or ThemeProvider and applies it to style.

  • Adds documentation examples for new hooks and styled and withStyles HOCs.

  • Improves performance of mapping theme by doing this once ThemeProvider is rendered instead of doing it each time cunsuming components are rendered.

  • Resolves accessing theme values from theme prop provided by withStyles (issue comment)

Closes #752

@artyorsh artyorsh added the 📱 Components components module-specific label Dec 19, 2019
@artyorsh artyorsh requested a review from 32penkin December 19, 2019 13:37
@artyorsh artyorsh self-assigned this Dec 19, 2019
@artyorsh artyorsh merged commit 67a8f8f into master Dec 20, 2019
@artyorsh artyorsh deleted the feat/theme-hooks branch December 20, 2019 12:28
@vomchik
Copy link

vomchik commented Dec 25, 2019

hi @artyorsh. Could you explain your motivation why you decide do not use useStylesSheet directly inside the function component?

const styles = StyleSheet.create(); looks a little bit udnersable becouse you dont use use prefix from hooks convetion.

Thanks.

@artyorsh
Copy link
Collaborator Author

artyorsh commented Dec 26, 2019

Hi @vomchik 👋

Before I start to explain the motivation, let me describe the goals of this function:

  • It should be able to work with eva theme variables (e.g color-primary-default)
  • It should use an actual theme and transform these variables to an actual theme values
  • It also should be compatible with other styles that we can use in StyleSheet.

My motivation is to create an API similar that we have out of the box in React Native (I mean similar to StyleSheet.create), since the way of declaring styles outside of the component is a common practice. Also since this function uses theme context, I see it being a hook so it should have use prefix.

I understand that this might be a little confusing to use use functions outside of the functional component, but currently, this feature is under internal testing, so if you have any suggestions we're able to discuss.

Thanks for the review 👍

@vomchik
Copy link

vomchik commented Jan 3, 2020

Happy New Year!

I see, thanks for the answer.

i have a few suggestions:

  1. Let's rename useStyleSheet to StyleSheetCreator or StyleSheetFactory which returns a new hook, then we can use this hook inside a component as is.
  2. Let's continue to use StyleSheet API from RN and create a useStyle hook, in which we can pass RN style number or function with theme variables
const styles = StyleSheet.create({
  button: {
    flex: 1
  }
});

const Button = () => {
  const style = useStyle([
    styles.button,
    theme => ({
      color: theme["color-primary-default"]
    }),
    theme => ({
      borderRadius: theme["border-radius"]
    })
  ]);

  return <View style={style}></View>
};

Currently, I use own style hook with next API

const Picker = () => {
  const text = useStyle(theme => ({
    color: theme["system-color-control-default"]
  }));

  return <Text style={[styles.button, text]}>Done</Text>;
};

@fvieira
Copy link

fvieira commented Jan 5, 2020

I really like this new useStyleSheet hook, but I agree with @vomchik that the naming needs a bit more thought, after all the useStyleSheet isn't exactly a hook (it returns an object with a create function that is the actual hook). This is confusing since this "hook" can be used outside the component, and the real hook (the create function) cannot.

Also, if you use the react-hooks eslint plugin, it's going to complain about the use of the useStyleSheet "hook" outside of a component, which is annoying.

For now I've followed @vomchik idea and created a function to turn this API into something a bit less confusing for me, I'll leave it here for inspiration:

// In a utils.ts file
import { StyleSheet } from 'react-native';
import { useStyleSheet } from '@ui-kitten/components';

export function makeUseStyles<T>(
  styles: StyleSheet.NamedStyles<T>,
): () => StyleSheet.NamedStyles<T> {
  // eslint-disable-next-line react-hooks/rules-of-hooks
  const UIKStyleSheet = useStyleSheet(styles);
  const useStyles = () => UIKStyleSheet.create();
  return useStyles;
}


// In a component's file

import { makeUseStyles } from '@app/utils';

const useStyles = makeUseStyles({
  text: {
    color: 'text-hint-color',
  },
});

function Label({children}) {
  const styles = useStyles();

  return (
    <Text style={styles.text}>
      {children}
    </Text>
  );
}

@artyorsh
Copy link
Collaborator Author

artyorsh commented Jan 17, 2020

@fvieira @vomchik usedYourIdeas in #833. You can join the discussion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📱 Components components module-specific
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hooks
4 participants