-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Remove overlay 70% opacity to allow more flexibility #629
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
|
@Famousfrank |
|
@osdnk currently, If I supply a flat black background color to the overlay The result will be that the overlay has a black background color with a 70% opacity. If I supply My suggestion is to remove the 0.7 setting (set it to 1), to allow people to set the background color themselves. For instance, if I want a red background at 90% opacity, I can just give Given to the plugin: Given to the plugin: The red is to make it even more clear what is happening. In my case I need a white 90% overlay, and while I can live with 70%, being able to set it at 90% will just make it a bit nicer. |
kmagiera
left a comment
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.
Can you please take a look at my inline comment?
Also, this seem to be a relatively small change and I'd prefer to keep such changes backward compatible. One suggestion would be to change default color from black to rgba(0,0,0,0.7) (semitransparent black) which should yield a similar end effect unless you set overlayColor which I think is much less frequent.
| const overlayOpacity = this._openValue.interpolate({ | ||
| inputRange: [0, 1], | ||
| outputRange: [0, 0.7], | ||
| outputRange: [0, 1], |
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.
not sure if there is point in interpolating [0,1] to [0,1]. Would see this only making sense if we support overshooting (such that _openValue is greater than 1) but I don't think we do.
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.
As far as I understand, this is just used for the animation right? This is basically what forced the 70% opacity. If the goal would be to allow people to define their own overlay background (even solids) this needs to be 1. Am I misunderstanding 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.
Also, this seem to be a relatively small change and I'd prefer to keep such changes backward compatible. One suggestion would be to change default color from black to rgba(0,0,0,0.7) (semitransparent black) which should yield a similar end effect unless you set
overlayColorwhich I think is much less frequent.
Good point! Updated the default color.
kmagiera
left a comment
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.
Sweet, thanks 🙌
…n#629) ## Purpose Remove the forced 70% opacity overlay to allow more flexibility - for instance if you want 90% instead. ## Use When declaring overlay background, you could feed it `rgba(255, 255, 255, 0.9)` to achieve a white overlay that is only slightly transparent. (cherry picked from commit 0f31a2f)
…n#629) ## Purpose Remove the forced 70% opacity overlay to allow more flexibility - for instance if you want 90% instead. ## Use When declaring overlay background, you could feed it `rgba(255, 255, 255, 0.9)` to achieve a white overlay that is only slightly transparent.

Purpose
Remove the forced 70% opacity overlay to allow more flexibility - for instance if you want 90% instead.
Use
When declaring overlay background, you could feed it
rgba(255, 255, 255, 0.9)to achieve a white overlay that is only slightly transparent.