-
Notifications
You must be signed in to change notification settings - Fork 739
Incubator.Gradient - new component #3795
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
Conversation
centerV?: boolean; | ||
}; | ||
|
||
// type GradientType = 'rectangle' | 'circle' | 'border'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hoped to fix the expect-error in the screen, kinda still want to keep this
@@ -0,0 +1,35 @@ | |||
import type {LinearGradientProps} from 'react-native-linear-gradient'; | |||
|
|||
type CommonGradientProps = Pick<LinearGradientProps, 'colors' | 'children'> & { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LinearGradientProps has start & end
props (link), I think the user should be able to control them.
Nice work with the angle
prop but playing with start & end
can give you more results (what I asked about if the gradient can start from another point)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have an example where this is useful? More to the point, if we ever move to react-native-gradients - it supports angle
and not start
& end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
example:
<View width={80} height={80}>
<LinearGradient
colors={[Colors.$backgroundPrimaryHeavy, Colors.$backgroundPrimaryMedium, Colors.$backgroundPrimaryLight]}
start={{x: 0.01, y: 0.02}}
end={{x: 0.9, y: 0.8}}
>
<View width={80} height={80} center>
<Text white>Hello</Text>
</View>
</LinearGradient>
</View>
As you can see the gradient as more of the backgroundPrimaryLight
since the start coordinates changed.
Does the react-native-gradients migration will be soon ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically we can get something very similar using a different colors
array (for example: const COLORS = [Colors.$backgroundPrimaryHeavy, Colors.$backgroundPrimaryMedium, Colors.$backgroundPrimaryMedium];
)
Either way I don't want to add a general support for start
and end
, if it'll be required we can add a specific type
for it (so we can deprecate it more easily).
Not sure we'll do it, it does't seem like it's very active, but it's only a few JS files, so we can more easily handle changes if we need to.
|
||
type CommonGradientProps = Pick<LinearGradientProps, 'colors' | 'children'> & { | ||
angle?: number; | ||
center?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The center alignment props only affect the children content, not the gradient.
These should be moved to a separate prop group like ContentAlignmentProps
with boolean values, WDYT ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't center
always affects the children?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it does, for me when I pass center
or centerV/H
to the gradient I expect the color of the gradient to be aligned also.
I leave it up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed the screen's text but I think the modifier is ok
const {type = 'rectangle', ...others} = props; | ||
|
||
useEffect(() => { | ||
if (LinearGradient === undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This useEffect
isn't relevant because the fallback is {default: () => null}
, not undefined
.
Am I missing something ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I'm reverting the change to LinearGradientPackage
since it affects SkeletonView
as well
src/incubator/gradient/index.tsx
Outdated
} | ||
}, []); | ||
|
||
if (!LinearGradient) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if (!LinearGradient)
check is irrelevant because LinearGradient
cannot be falsy (it's either a component or a function).
The fallback function () => null
will pass this check consider checking if (LinearGradient === null)
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed a little
@@ -16,24 +16,29 @@ import { | |||
View | |||
} from 'react-native-ui-lib'; | |||
|
|||
interface RadioGroupOptions { | |||
interface StateOptions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file includes changes that belong in a different PR.
Have you verified that these changes don't break other screens ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes are required for this PR, if I made these changed in another (clean) PR, how would we test them?
Tested a few screens
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, Iv'e left few comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 🎨
* Incubator.Gradient - new component * Oops * Revert LinearGradientPackage change * Improve screen
Description
Incubator.Gradient - new component
Ideally this would be using something other than
react-native-linear-gradient
, however I did not manage to get all the features to work consistently.Changelog
Incubator.Gradient - new component
Additional info
None