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

fix(AboutModal): provide a way to set the background using props #1940

Merged
merged 1 commit into from May 8, 2019

Conversation

@seanforyou23
Copy link
Member

seanforyou23 commented May 6, 2019

What: This PR removes the logoImageSrc and logoImageAlt props from about modal as they were never actually used. Beyond removing these dead API parts, I've replaced them with a single prop, bgImgSrc which I believe fulfills the original intent - which is making the background graphic on the right side of the modal configurable.

I've added a new example for AboutModal, one that illustrates how to set a custom background image.

The value passed for backgroundImageSrc is used to set the value for the css variable "--pf-c-about-modal-box__hero--sm--BackgroundImage". This is different than how we typically set CSS vars, and the need for it is described here #1919. In a nutshell, the need is driven by having assets served by a caching service or using generated asset filenames of which you don't know the exact name of a file, which in turn prevents you from setting this in a stylesheet.

Additional issues: #1919

Screen Shot 2019-05-06 at 3 33 55 PM

@seanforyou23 seanforyou23 requested review from dlabaj, dgutride, mcoker and tlabaj May 6, 2019
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented May 6, 2019

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 6, 2019

Codecov Report

Merging #1940 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1940      +/-   ##
==========================================
- Coverage   82.59%   82.59%   -0.01%     
==========================================
  Files         624      624              
  Lines        6878     6876       -2     
  Branches       93       93              
==========================================
- Hits         5681     5679       -2     
  Misses       1157     1157              
  Partials       40       40
Flag Coverage Δ
#patternfly3 84.89% <ø> (ø) ⬆️
#patternfly4 79.27% <100%> (-0.02%) ⬇️
#patternflymisc 95.68% <ø> (ø) ⬆️
Impacted Files Coverage Δ
...e/src/components/AboutModal/AboutModalContainer.js 88.88% <ø> (ø) ⬆️
...react-core/src/components/AboutModal/AboutModal.js 66.66% <ø> (-4.17%) ⬇️
...ore/src/components/AboutModal/AboutModalBoxHero.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 82f8002...09cbd42. Read the comment docs.

Copy link
Contributor

mcoker left a comment

👍

@seanforyou23 seanforyou23 requested a review from redallen May 6, 2019
Copy link
Contributor

dlabaj left a comment

Few comments. Other then that it looks good.

"--pf-c-about-modal-box__hero--sm--BackgroundImage": `url(${bgImgSrc})`
} : {};
return (
<div {...props} style={bgStyle} className={css(styles.aboutModalBoxHero, className)} />

This comment has been minimized.

Copy link
@dlabaj

dlabaj May 7, 2019

Contributor

What happens if it doesn't find the image?

This comment has been minimized.

Copy link
@seanforyou23

seanforyou23 May 8, 2019

Author Member

Good question, so if you're referencing a local module like in the example, and it can't be found, your IDE should notice this and throw an error. If you're providing a value like https://www.site.com/image.svg and that asset doesn't properly fetch, you'd get a standard 404 just as if you provide an inaccessible asset in any regular background definition, in which case you'd get a black background.

logoImageAlt: props => {
if (props.logoImageSrc && !props.logoImageAlt) {
return new Error('logoImageAlt is required when a logoImageSrc is specified');
brandImageAlt: props => {

This comment has been minimized.

Copy link
@jschuler

jschuler May 7, 2019

Collaborator

since brandImageSrc is already required, we can also simply set brandImageAlt to required.
Although i'm questioning whether the brand image should be required...

This comment has been minimized.

Copy link
@seanforyou23

seanforyou23 May 8, 2019

Author Member

For now I left it as required, can change at a later point if you like. The main focus for this PR is just to make the background image configurable.

@seanforyou23 seanforyou23 force-pushed the seanforyou23:about-modal-bg branch from 3f224c9 to 09cbd42 May 8, 2019
@seanforyou23 seanforyou23 requested review from jschuler, dlabaj and mcoker May 8, 2019
@dlabaj
dlabaj approved these changes May 8, 2019
@tlabaj
tlabaj approved these changes May 8, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

@jschuler jschuler merged commit 4ad711d into patternfly:master May 8, 2019
2 checks passed
2 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.