Skip to content

Conversation

@aravindballa
Copy link

Fixes #445

Added a new prop bgLighten to Base so that it supports lightening images.

Screenshot:

Screenshot_5

Copy link
Contributor

@kitten kitten left a comment

Choose a reason for hiding this comment

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

Looks pretty great! 😬 I’m wondering whether it’d make sense to unify these props so that they don’t conflict as much (maybe negative numbers for darken? Or custom overlay colours?) cc @kenwheeler

@aravindballa
Copy link
Author

Custom overlay colours would be a great one. Something like bgOverlayColour and bgOverlayIntensity.

@kenwheeler
Copy link
Contributor

Lets do that. That will be tight

@aravindballa
Copy link
Author

Please review and check whether we can continue like this or not.
@kenwheeler @philpl

return convertedFontSize;
};

const hexToRGB = function (hex) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of maintaining this piece of code ourselves it should probably use a helper. Polished has some excellent ones and we won't need to pull in the entire library ✨

https://polished.js.org/docs/#transparentize

or

https://polished.js.org/docs/#parsetorgb

bgImage,
bgDarken,
bgOverlayColour,
bgOverlayIntensity,
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't call this prop "intensity" as that would refer to the saturation of the colour: https://www.uwgb.edu/heuerc/2D/ColorTerms.html

Instead I would just call this "opacity" or "opacify" or "transparentize" depending on what it'll do.

Copy link
Author

Choose a reason for hiding this comment

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

Its bgTransparentize now ✨

@aravindballa
Copy link
Author

Can you verify if I did it right? @philpl
cc @kenwheeler

@aravindballa
Copy link
Author

@kitten @kenwheeler Can you check?

@ebrillhart
Copy link
Contributor

Hey @aravindballa thank you so much for your work on this! I am closing this since the functionality was added in #531 (along with some other changes we needed), which will be merged soon.

@ebrillhart ebrillhart closed this Aug 22, 2018
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.

4 participants