Deprecated props & excessive CSS rules

[r007]

Hi @timhagn

There are a couple props in your index.js

gatsby-background-image/src/index.js

Line 346 in e495fa6

placeholderStyle: PropTypes.object,

  style: PropTypes.object,
  imgStyle: PropTypes.object,
  placeholderStyle: PropTypes.object,

PropTypes.object is deprecated, please use PropTypes.style instead.

Also, there are many excessive CSS rules for every background. Here for example

gatsby-background-image/src/StyleUtils.js

Line 68 in e495fa6

transition: opacity 0.5s;

background-size is an old property which is widely supported by all modern browsers, no prefixes are needed. transition-delay - only -webkit prefix is needed. transition - only -webkit prefix is needed. So need to refactor CSS rule from this:

.background-image {
  -webkit-background-size: ${backgroundSize};
  -moz-background-size: ${backgroundSize};
  -o-background-size: ${backgroundSize};
  background-size: ${backgroundSize};
  -webkit-transition-delay: ${transitionDelay};
  -moz-transition-delay: ${transitionDelay};
  -o-transition-delay: ${transitionDelay};
  transition-delay: ${transitionDelay};
  -webkit-transition: opacity 0.5s;
  -moz-transition: opacity 0.5s;
  -o-transition: opacity 0.5s;
  transition: opacity 0.5s;
}

to this:

.background-image {
  background-size: ${backgroundSize};
  -webkit-transition-delay: ${transitionDelay};
  transition-delay: ${transitionDelay};
  -webkit-transition: opacity 0.5s;
  transition: opacity 0.5s;
}

Hope this helps.

[timhagn]

Hi @r007,

thanks for your input, gonna dig into it tomorrow,
cause today my wife and I have our 12th relationship anniversary,
so no coding for me today : ).

Read ya tomorrow,

Tim.
P.S.: The current branch at the moment is 0.2.8-alpha. There the vendor-prefixes get added automatically, see line 111 in StyleUtils.js over there.

[timhagn]

master is now again the current branch with today's release of 0.2.8-beta (after merging in #23).
Might you elaborate on the "PropTypes.style", as I couldn't find such a declaration in props-types?

[timhagn]

Hi @r007,

have you had the chance to look into and elaborate on PropTypes.style?
And about the excessive CSS-rules - especially when we now look into just passing this.backgroundStyles and / or the style={{}} prop, see my latest comment in #20 - which rules should we stop "vendorizing" (and such stop including older browsers) in the first place in your opinion?

And I'm thinking bout removing placeholderClassName and placeholderStyle altogether,
cause methinks it might get confused with the existing classId or further changes also mentioned in #20, or what do you think?

Thanks again for raising this! Should we leave this open as an issue on it's own, or rather integrate the discussion in #20, through such new developments?

[timhagn]

Hi @r007, in the current version 0.2.8 the two props placeholderClassName and placeholderStyle are removed for the moment. Though I'd still really want to know where you found PropTypes.style ; ).

[timhagn]

Oh, and we are now at 0.2.9 (yeay, SemVer ; ) cause of issue #24.

[timhagn]

Hi @r007,
looked a little further into the propTypes.style matter, and might it just be
that you thought about React Native PropTypes?
The only package apart from theirs where I could find a style propType was react-style-proptype oO.

[timhagn]

Hi @r007, again ; ).
No time delving into this matter at the moment or anymore?
For the latest updates, please read my comment in #20.

[r007]

Hi @timhagn

Sorry for late reply. Congratulations on your anniversary. Yes, my bad, propTypes.style doesn't exist. But you can use this instead:

PropTypes.objectOf(PropTypes.object)

It will remove deprecated warning. Agree that it's probably a good idea to remove placeholder props. What about CSS rules, background-size don't needs to be prefixed. Also, -moz and -o prefixes can be removed.

[timhagn]

Hi @r007,

no problem, thanks for replying and the nice anniversary wishes (was a great one : )!

Might I ask, where you got the idea about PropTypes.object for styles being deprecated from?
Really interested, no offense meant! But I searched "far and wide" and could neither find any notion about it being deprecated (except for React Native) nor do I get any deprecation warning in any browser I tried oO. And sorry to say, using your proposed solution gives me a

Warning: Failed prop type: Invalid prop `style.backgroundSize` of type `string` supplied to `BackgroundImage`, expected `object`.

when adding a simple style prop like such in gbitest

style={{
  backgroundSize: 'contain',
}}

With the CSS rules I'm just trying to figure out, how much Gatsby itself precompiles / autoprefixes on it's own - it stripped the superfluous -ms-* prefixes on every build, gotta try it with newer CSS4 ones.
Maybe just add a Boolean prop for prefixing?

Still thanks for the input : )!

[r007]

Hi @timhagn,

I guess it throws ESLint, here, check this out: forbid-prop-types. Here for example, when I am trying to declare PropTypes.object in my own project:

But this particular warning gives me PHPStorm in your project when I hover over the propType declaration:

I don't remember the exact declaration, I typed by memory. Here's how style prop is declared in one of my projects: style: PropTypes.oneOfType([PropTypes.object]).

Would be nice to have some kind of ESLint preset in your project. Airbnb for example.

What about prefixing - no, prop is not needed I think.

[timhagn]

Hi @r007,

ah, OK, now I see - haven't thought about further linting rules (was quite content with the default
IntelliJ IDEA ones - JetBrains is great ; ). Seems to be a problem akin to this one and according to your second image PropTypes.object equally seems marked as deprecated / "forbidden" in the className: PropTypes.oneOfType([...]) prop : /. Thinking bout either using react-style-proptype, though I have no idea how that would perform with ESLint...

Would be nice to have some kind of ESLint preset in your project. Airbnb for example.

Feel free to open a PR : ). Opened a hellhole with issue #20, as it seems % |.

[timhagn]

Finally 0.3.0 reached npmjs / yarnpkg : ).

I went with PropTypes.oneOfType([PropTypes.object, PropTypes.array]) for the style={{}} prop for the moment, though am still open for suggestions.
The superfluous vendor prefixes are gone, according to caniuse.com even most of the transition-* styles don't need em.
Might we close this issue and open another one for ESLint and / or TypeScript Definitions?

[r007]

Hi @timhagn

Okay, thanks. I think it's good enough, good job. Yes, I'm telling you, prefixes are not needed simply because most of the browsers support them without any problems. Minimal requirement for Gatsby is ES5 as much as I know. These are all quite modern browsers.

I close it, will open another issue/PR for ESLint :)