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

Linear gradient #80

Merged
merged 13 commits into from
May 21, 2021
Merged

Linear gradient #80

merged 13 commits into from
May 21, 2021

Conversation

cmaycumber
Copy link
Contributor

Adds a linear gradient component to solve: #73

@nandorojo
Copy link
Owner

I'm thinking we should move the gradient into its own package so that people who aren't using it don't need to install the extra dependency. This is what I did for Moti's skeleton.

@cmaycumber
Copy link
Contributor Author

I'm thinking we should move the gradient into its own package so that people who aren't using it don't need to install the extra dependency. This is what I did for Moti's skeleton.

Everything should be in the gradient package. Let me know if I missed something or maybe accidentally exported it from core. I'll take a look through it again later today.

@nandorojo
Copy link
Owner

Looks like it's exported in the main Dripsy package, but it should require installing @dripsy/gradient

@cmaycumber
Copy link
Contributor Author

Ahh I gotcha I'll make the change

@nandorojo
Copy link
Owner

Also, the dependency for Dripsy/core should have a specified version instead of *

@nandorojo
Copy link
Owner

We should probably change this to use expo-module prepare for the build step too, although I want to see what's up with #86. We might need a custom webpack config for web apps, like this one from moti: https://moti.fyi/web

@cmaycumber
Copy link
Contributor Author

We should probably change this to use expo-module prepare for the build step too, although I want to see what's up with #86. We might need a custom webpack config for web apps, like this one from moti: https://moti.fyi/web

Do you think we should add this before merging in or do you still want to experiment a bit more with how expo-module's is affecting the web projects?

@nandorojo
Copy link
Owner

We can leave it with Bob for now to keep comparing, even though the expo module scripts is merged to master in the other packages.

@@ -7,6 +7,7 @@ import {
DripsyProvider,
Container,
Theme,
LinearGradient,
Copy link
Owner

Choose a reason for hiding this comment

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

This should come from the unique package btw

Add `stretch`, error messages
@nandorojo
Copy link
Owner

Made some changes, last thing is so fix the merge conflicts and update the example

@nandorojo
Copy link
Owner

https://theme-ui.com/migrating/ migration guide here

@nandorojo
Copy link
Owner

Seems like the biggest breaking change to deal with is the fact that emotion upgrades from 10 -> 11. Besides that, it's the JSX pragma syntax, and then some small stuff.

@cmaycumber
Copy link
Contributor Author

https://theme-ui.com/migrating/ migration guide here

Sweet this should be helpful. I'll take a look at this tonight.

@nandorojo
Copy link
Owner

One seemingly large breaking change is that the colors in the useThemeUI hook has changed to rawColors... 😬

I assume this is due to CSS variables? I'm not sure. Maybe we can just override that by reassigning rawColors: colors to start.

@nandorojo
Copy link
Owner

Sorry not sure why I put those theme-ui notes on this PR. Got it mixed up.

@nandorojo
Copy link
Owner

This PR is good to go, just need to fix the import in the example, make sure the gradient is not exported by the main package, and fix the remaining merge conflicts that GitHub is upset about.

@cmaycumber
Copy link
Contributor Author

This PR is good to go, just need to fix the import in the example, make sure the gradient is not exported by the main package, and fix the remaining merge conflicts that GitHub is upset about.

Will do.

@cmaycumber
Copy link
Contributor Author

@nandorojo This should be gtg. Let me know.

@nandorojo
Copy link
Owner

Cool thanks. Thoughts on exporting it as a named variable rather than default from dripsy/gradient?

@cmaycumber
Copy link
Contributor Author

Cool thanks. Thoughts on exporting it as a named variable rather than default from dripsy/gradient?

I'd prefer that. I think that will help keep things consistent.

@cmaycumber
Copy link
Contributor Author

Do you have a preference over just Gradient or LinearGradient?

@nandorojo
Copy link
Owner

Was just thinking that.

LinearGradient is more clear, since that's what it does (no radial, etc). Also consistent with the expo naming. It also sounds lamer, though.

I prefer Gradient. I'll sit on it for a few minutes but I say let's go with that. Sounds cleaner.

@cmaycumber
Copy link
Contributor Author

I prefer Gradient. I'll sit on it for a few minutes but I say let's go with that. Sounds cleaner.

Cool, I agree and it's nice that it's the name as the package name.

@nandorojo
Copy link
Owner

Let's stick with that

@nandorojo nandorojo changed the base branch from master to no-ssr May 20, 2021 20:23
@nandorojo
Copy link
Owner

Pointing this to the v2 branch

@nandorojo nandorojo changed the base branch from no-ssr to master May 20, 2021 22:57
@nandorojo nandorojo merged commit 030b06d into nandorojo:master May 21, 2021
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.

2 participants