fix(AboutModal): provide a way to set the background using props - #1940
Conversation
Sorry, something went wrong.
|
PatternFly-React preview: https://1940-pr-patternfly-react-patternfly.surge.sh |
Sorry, something went wrong.
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Sorry, something went wrong.
mcoker
left a comment
There was a problem hiding this comment.
👍
Sorry, something went wrong.
dlabaj
left a comment
There was a problem hiding this comment.
Few comments. Other then that it looks good.
Sorry, something went wrong.
| "--pf-c-about-modal-box__hero--sm--BackgroundImage": `url(${bgImgSrc})` | ||
| } : {}; | ||
| return ( | ||
| <div {...props} style={bgStyle} className={css(styles.aboutModalBoxHero, className)} /> |
There was a problem hiding this comment.
What happens if it doesn't find the image?
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| logoImageAlt: props => { | ||
| if (props.logoImageSrc && !props.logoImageAlt) { | ||
| return new Error('logoImageAlt is required when a logoImageSrc is specified'); | ||
| brandImageAlt: props => { |
There was a problem hiding this comment.
since brandImageSrc is already required, we can also simply set brandImageAlt to required.
Although i'm questioning whether the brand image should be required...
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
tlabaj
left a comment
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
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