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

Add makeStyles function #109

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add makeStyles function #109

wants to merge 2 commits into from

Conversation

matinzd
Copy link

@matinzd matinzd commented Sep 20, 2021

Changes

  • Change textVariants to typography for better coherence
  • Add makeStyles function to create hook

@ghost ghost added the cla-needed label Sep 20, 2021
@matinzd
Copy link
Author

matinzd commented Sep 20, 2021

@JoelBesada Couldn't sign the CLA. Rate limit exceeded.

@matinzd matinzd changed the title change textVariants to typography Add makeStyles function Sep 20, 2021
Copy link

@mblarsen mblarsen left a comment

Choose a reason for hiding this comment

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

I like the idea. It is a good way to use traditional styles like syntax.

However, I think this PR will never pass as long as you make the typography change at the same time. I strongly suggest that you split this into two PRs.

I'm not a contributor so you don't have to take my word for anything, just a friendly suggestion.

@@ -12,7 +12,7 @@ and adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).
* Add 2-dimensional breakpoints [#70](https://github.com/Shopify/restyle/pull/70) by [@Johan-duitot](https://github.com/Johan-dutoit)

## 1.3.1 - 2020-10-26
* Silently ignore any errors caused by missing a variant definition in the theme (e.g. textVariants), to preserve backwards compatibility. [#64](https://github.com/Shopify/restyle/pull/64) by [@jonogreenz](https://github.com/jonogreenz)
* Silently ignore any errors caused by missing a variant definition in the theme (e.g. typography), to preserve backwards compatibility. [#64](https://github.com/Shopify/restyle/pull/64) by [@jonogreenz](https://github.com/jonogreenz)

Choose a reason for hiding this comment

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

Perhaps leave old CHANGELOG entries untouched

Comment on lines 36 to 42
backgroundColor="mainBackground"
paddingVertical="xl"
paddingHorizontal="m">
paddingHorizontal="m"
>
<Text variant="header">Welcome</Text>
<Box
flexDirection={{

Choose a reason for hiding this comment

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

Generally avoid reformatting parts of code that you are not making any changes in.

It makes it really hard to spot the actual changes.

Comment on lines +304 to +306

const useStyles = makeStyle<YourTheme>(theme => ({
container: {

Choose a reason for hiding this comment

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

Suggested change
const useStyles = makeStyle<YourTheme>(theme => ({
container: {
const useStyles = makeStyles<YourTheme>(theme => ({
container: {

@mattfrances
Copy link
Contributor

Apologies for the long delay, is this PR still relevant?

@mblarsen
Copy link

Apologies for the long delay, is this PR still relevant?

I find it really useful, but don't know if @matinzd is still interested

@matinzd
Copy link
Author

matinzd commented Mar 30, 2023

Hey, I am still interested in having this function and it makes components much cleaner. I will try to rebase the PR and make some changes to the function as it has some typescript issues.

@chrism1996
Copy link

any updates in this feature?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants