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.

Sign up for GitHub

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

Jump to bottom

feat(gatsby-image): Add customProps as catchall #25283

Closed
wants to merge 3 commits into from
Closed

feat(gatsby-image): Add customProps as catchall #25283

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2a48a48
Add customProps as catchall for gatsby-image props
RetroCraft Jun 25, 2020
b9d7154
filter out props from customProps
RetroCraft Jun 25, 2020
053db18
Define prop for custom props instead of using spread
RetroCraft Jun 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/gatsby-image/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,7 @@ class Image extends React.Component {
itemProp,
loading,
draggable,
tagProps = {},
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's be extra explicit here

Suggested change
tagProps = {},
imageTagProps = {},

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not for the img element though? It's for the div(default tag) wrapper element. My review suggested wrapperAttrs or attrProps which is more clear what the purpose of this props is(attributes for passing to the top-level element). We have lots of "image"/"tag"/"props" keywords in the file already, so I definitely think it's more clearer in all that keyword noise to be more specific it's about attributes.

} = convertProps(this.props)

const shouldReveal = this.state.fadeIn === false || this.state.imgLoaded
Expand Down Expand Up @@ -486,6 +487,7 @@ class Image extends React.Component {
}}
ref={this.handleRef}
key={`fluid-${JSON.stringify(image.srcSet)}`}
{...tagProps}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be the first prop, place it above all others, otherwise it introduces the possibility to override all prior props for this component.

>
{/* Preserve the aspect ratio. */}
<Tag
Expand Down Expand Up @@ -601,6 +603,7 @@ class Image extends React.Component {
style={divStyle}
ref={this.handleRef}
key={`fixed-${JSON.stringify(image.srcSet)}`}
{...tagProps}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as other comment, order matters, place this above all other props.

>
{/* Show a solid background color. */}
{bgColor && (
Expand Down