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

refactor(components): update useStyleSheet hook #833

Merged
merged 4 commits into from
Jan 21, 2020

Conversation

artyorsh
Copy link
Collaborator

@artyorsh artyorsh commented Jan 17, 2020

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

Short description of what this resolves:

Creates a new service for creating themed styles and updates useStyleSheet hook due to Rules of Hooks

Discussed by users in #782
Closes #831

New syntax:

import React from 'react';
- import { Layout, useStyleSheet } from '@ui-kitten/components';
+ import { Layout, StyleService, useStyleSheet } from '@ui-kitten/components';

export const UseStyleSheetSimpleUsageShowcase = () => {
-  const styles = StyleSheet.create();
+ const styles = useStyleSheet(themedStyles);

  return (
    <Layout style={styles.container} />
  );
};

- const StyleSheet = useStyleSheet({
+ const themedStyles = StyleService.create({
  container: {
    flex: 1,
    backgroundColor: 'color-success-default',
  },
});

@artyorsh artyorsh self-assigned this Jan 17, 2020
@artyorsh artyorsh mentioned this pull request Jan 17, 2020
2 tasks
@fvieira
Copy link

fvieira commented Jan 17, 2020

Thanks for hearing our feedback, @artyorsh.
This is much better, definitely.

I still have a small gripe that we're using StyleSheet.create to return not an actual stylesheet (the styles variable) as the name implies, but a hook.
I understand that we're trying to keep the StyleSheet.create that RN users are used to, but users are also used to it returning a styles variable, so this might actually work against us, they might just change the StyleSheet import from react-native to @ui-kitten/components and then wonder why the code is not working.

My recommendation would be to rename it to StyleSheet.createHook. This way, if they just change the import, the code breaks as it should (or Typescript complains the create method does not exist), they can use autocomplete to find out the name of the new method (createHook), which in turn acts as a reminder that a hook is returned, not the styles themselves.

@artyorsh
Copy link
Collaborator Author

artyorsh commented Jan 17, 2020

@fvieira Well, after small brainstorming we decided to rename a service, not a function. In general, this create method does absolutely nothing, unlike RN StyleSheet which does validation and styles registering. We just can't use it because it does not support style preprocessing currently. But I think it is useful to have it in order to support the syntax we used to and may be preprocessing if it becomes non-experimental feature. See more details

The main functionality is back again in useStyleSheet which does all processing stuff.

I've updated usage example in the PR description

@artyorsh artyorsh changed the title refactor(components): deprecate useStyleSheet hook refactor(components): update useStyleSheet hook Jan 17, 2020
@fvieira
Copy link

fvieira commented Jan 17, 2020

@artyorsh I think this new nomenclature fixes all problems with the previous ones, I see nothing misleading about it now!

@artyorsh artyorsh merged commit 8b33c91 into master Jan 21, 2020
@artyorsh artyorsh deleted the refactor/use-style-sheet branch January 21, 2020 07:59
@vomchik
Copy link

vomchik commented Jan 24, 2020

@artyorsh Nice! I really like a new approach. Thanks.

I see only one flaw, it is a typing for theme variables. color-primary-default looks like a magic string. But I guess we can improve it 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.

useStyleSheet hook doesn't follow Hook rules
4 participants