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

Cover Image: Add configurable overlay color #10291

Merged
merged 3 commits into from Oct 4, 2018
Merged

Conversation

@mcsf
Copy link
Contributor

@mcsf mcsf commented Oct 2, 2018

Description

cover-image-overlay-color

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
@mcsf mcsf requested a review from mtias Oct 2, 2018
@mcsf mcsf force-pushed the update/cover-image-overlay-color branch 2 times, most recently from 6778574 to d23daa6 Oct 3, 2018
@mcsf mcsf force-pushed the update/cover-image-overlay-color branch from d23daa6 to 40f80bb Oct 3, 2018
@aduth aduth self-requested a review Oct 3, 2018
const style = backgroundImageStyles( url );
if ( ! overlayColorClass ) {
style.backgroundColor = customOverlayColor;

This comment has been minimized.

@aduth

aduth Oct 3, 2018
Member

backgroundImageStyles can return undefined which would throw an error here. Can that happen?

This comment has been minimized.

@mcsf

mcsf Oct 3, 2018
Author Contributor

Good spot. Addressed.

const classes = classnames(
className,
dimRatioToClass( dimRatio ),
{
'has-background-dim': dimRatio !== 0,
'has-parallax': hasParallax,
[ `has-${ contentAlign }-content` ]: contentAlign !== 'center',
[ overlayColorClass ]: !! overlayColor,

This comment has been minimized.

@aduth

aduth Oct 3, 2018
Member

This can just be an argument to classnames, omitted if falsy, if overlayColorClass can be the boolean measure of if a custom color is assigned. Above we consider it to be (if ( ! overlayColorClass ) {), here we use overlayColor.

Copy link
Member

@tofumatt tofumatt left a comment

This is cool! I just have a few comments, not really a full review.

<PanelBody title={ __( 'Cover Image Settings' ) }>
<ToggleControl
label={ __( 'Fixed Background' ) }
checked={ !! hasParallax }

This comment has been minimized.

@tofumatt

tofumatt Oct 3, 2018
Member

Isn't this a boolean? Could this be simplified to checked={ hasParallax }?

This comment has been minimized.

@mcsf

mcsf Oct 3, 2018
Author Contributor

Technically this isn't part of the changes (the no-whitespace view is handy), but addressed.

<RichText
tagName="h2"
value={ title }
onChange={ ( value ) => setAttributes( { title: value } ) }

This comment has been minimized.

@tofumatt

tofumatt Oct 3, 2018
Member

I dunno if we have a policy on inline functions as props but my brain always thinks "technically we're creating a new function on every render". It's also less easy to test/identify in stack traces/etc. when it's an inline anonymous function.

All of that doesn't really matter here, but it's just a thought. I like being able to see what functions exist in a component without having to go way down into props.

</NitpickOfTheCentury>

🤷‍♂️

This comment has been minimized.

@mcsf

mcsf Oct 3, 2018
Author Contributor

Related discussion on inline functions, at least when it comes to performance:
#5495 (comment)

className="wp-block-cover-image-text"
placeholder={ __( 'Write title…' ) }
value={ title }
onChange={ ( value ) => setAttributes( { title: value } ) }

This comment has been minimized.

@tofumatt

tofumatt Oct 3, 2018
Member

This is actually an example of what I referred to above in my nitpick of the year actually: looks like this is the exact same function as another anonymous, inline one passed to a prop above. Might as well combine 'em.

@mcsf
Copy link
Contributor Author

@mcsf mcsf commented Oct 3, 2018

af2154f should take care of all the feedback.

@mcsf
Copy link
Contributor Author

@mcsf mcsf commented Oct 3, 2018

Note that E2E test failure comes from master.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

I left a review with some possible enhancements but things tested well for me. I did not found any blocker.

checked={ hasParallax }
onChange={ toggleParallax }
/>
<PanelColorSettings

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Oct 4, 2018
Member

PanelColorSettings includes a small rectangle with the color preview. As a possible improvement, we may apply the opacity CSS to that rectangle so the color preview is more reliable.

@mtias mtias self-requested a review Oct 4, 2018
@mtias
mtias approved these changes Oct 4, 2018
@mcsf mcsf merged commit a01b009 into master Oct 4, 2018
2 checks passed
2 checks passed
codecov/project 49.59% (+0.16%) compared to d6af165
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mcsf mcsf deleted the update/cover-image-overlay-color branch Oct 4, 2018
@mtias mtias added this to the 4.0 milestone Oct 9, 2018
@Soean Soean mentioned this pull request May 13, 2020
0 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.