`background-*` passthrough or singular pseudo-Element styling? #20

timhagn · 2019-03-27T18:06:01Z

Hi there,
wanted to ask all who invested some of their time into this project a question, which came up through @r007's PR #17 (now integrated in the just published 0.2.8-alpha3):

How should gatsby-background-image handle background styling?
There are three ways coming to my mind:

Pass only specific CSS-properties like the existing backgroundSize, backgroundRepeat and (with Add background-position property to background image #17) backgroundPosition parsed with getBackgroundStyles() through to the the pseudo-Elements.
Just pass every background-* CSS Property parsed with getBackgroundStyles() through to the pseudo-Elements.
As I just ascertained, document.styleSheets contains every ::before & ::after Element written as they are in CSS / constructed by styled-components and suchlike CSS-in-JS solutions written like .frjXhE::after, .frjXhE::before in a CSSStyleRule's selectorText when created by styled-components thus way:

...
  &:after, &:before {
     background-position: center;
  }
...

(It works nearly the same for media-queries, by the way : )!

What do you think? Which should be the way to go in your opinion?

Thanks in advance,

Tim.

@r007 @ambethia @einarbirgir @milanito @nshoes @petrpacas @EricSSartorius @apaulau @drobinhood @adam-wheatley @itwasmattgregg @AlmeroSteyn - and everyone wanting to speak up on this question : )

EDIT: Fixed the structure of my first sentence and added the new published 0.2.8-alpha2 version.
EDIT2: Changed 0.2.8-alpha2 to current version 0.2.8-alpha3.
EDIT3: Changed wrong usage of "props" in 1. to a better explanation, also see my comment below.

The text was updated successfully, but these errors were encountered:

petrpacas · 2019-03-28T10:57:22Z

Well, the question is whether to follow gatsby-plugin-image, or start diverging.

I personally prefer not styling through props, it's as if mixing two things...
(But that's because I like using styled-components)

How about supporting both ways?

timhagn · 2019-03-28T15:08:07Z

Oh, I misspelled myself, will edit it in the original question - no further props were added.
The three CSS-styles mentioned in the original post in alternative 1 are getting parsed through
getBackgroundStyles() as well and weren't added as props % )!

Wanted to stay in sync with gatsby-image as much as possible.

timhagn · 2019-03-28T21:53:05Z

Current version now is 0.2.8-alpha4 which should bring the transitions back
"on par" with gatsby-image.

apaulau · 2019-03-29T06:55:26Z

Hi, @timhagn

I think it's not a big problem since it's possible to attach some class name to BackgroundImage component and style ::after and ::before pseudo-elements through it in any way (like simple css, css modules or css-in-js). And actually I'm using it this way on my project with css modules and have no problems.

By the way I don't really like the idea of passing css through the props since we already have css :)

timhagn · 2019-03-29T15:17:54Z

Hi @apaulau!

Thanks for your input, greatly appreciated!
Though to clear up the misconception (EDIT3 in the Original post),
I definitely don't have the intention to pass CSS through props ; )!

Point 1 only concerns specific background-* styles, which directly have to be passed through to the pseudo-elements, like the 3 ones mentioned (and already parsed in), which I miss-defined as props
in the first version of my initial issue-post ; ).

Right now (at least when I try it in gbitest) one has to use !important so that styles directly get applied from CSS-in-JS (without using a wrapper around it and specifically targeting
.gatsby-background-image-[YOUR_ID] that is), as well as in media queries (my initial TODO)

and I don't like this behavior much ^^.

But maybe I'm just overthinking it oO?

EDIT: Clarification of style-concerns and addition of .gatsby-background-image-[YOUR_ID].

r007 · 2019-03-31T13:30:15Z

Hi @timhagn,

I think all background-related properties should be parsed with getBackgroundStyles() anyways. However, leave the opportunity to override style using props.

Let's say move all

backgroundPosition
backgroundSize
backgroundColor
backgroundRepeat

etc.

Into one single style prop. Then I'll be able to simply write:

<Background
  fluid={image}
  style={{
    backgroundPosition: 'center center',
    backgroundSize: '100% 50%',
    backgroundAttachment: 'scroll',
    backgroundOrigin: 'border-box'
  }}
>
   Text
</Background>

Instead of specifying every single property (which is kind of annoying):

<Background
  fluid={image}
  backgroundPosition: 'center center'
  backgroundSize: '100% 50%'
  backgroundAttachment: 'scroll'
  backgroundOrigin: 'border-box'
>
   Text
</Background>

Hope this makes sense.

timhagn · 2019-04-01T02:51:28Z

Hi @r007,

thanks for the input, though to clear up the misconception again (EDIT3 in the Original post),
I definitely don't have the intention to pass CSS through props ; )!
As I just merged your PR in, I gather you already know that backgroundPosition get's read through getBackgroundStyles() and just backgroundColor (which was a prop taken over from gatsby-image which defaults to lightgray if just beeing present, so it's Boolean) already is a prop.

From the rest you were writing, might I guess you'd prefer option 2 (Just pass every background-* CSS Property parsed with getBackgroundStyles() through to the pseudo-Elements), while still being able to overwrite them through an outer .gatsby-background-image-[YOUR-ID] class?

Or would you want to include option 3 from above, too?

Thanks again and in advance,

Tim.

r007 · 2019-04-01T06:31:29Z

@timhagn, yes option 2. Outer class .gatsby-background-image-[YOUR-ID] is an overkill, at least for me. Why would I needed it if I can simply assign class using className or id and then target it in my css.

<BackgroundImage id="Hero" className="Hero">
   Some content
</BackgroundImage>

.Hero:before,
.Hero:after {
   background-position: center;
}

It's basically the same. But like I said, would be nice to have ability to override styles through inline style. Like in the example above:

<Background
  fluid={image}
  style={{
    backgroundPosition: 'center center',
    backgroundSize: '100% 50%',
    backgroundAttachment: 'scroll',
    backgroundOrigin: 'border-box'
  }}
>
   Text
</Background>

Option 3 is not really needed I think. It would be too computationally intensive to parse every :before & :after selector. So options 1 & 2 I think...

apaulau · 2019-04-01T07:09:44Z

Hi @apaulau!

Thanks for your input, greatly appreciated!
Though to clear up the misconception (EDIT3 in the Original post),
I definitely don't have the intention to pass CSS through props ; )!

Point 1 only concerns specific background-* styles, which directly have to be passed through to the pseudo-elements, like the 3 ones mentioned (and already parsed in), which I miss-defined as props
in the first version of my initial issue-post ; ).

Right now (at least when I try it in gbitest) one has to use !important so that styles directly get applied from CSS-in-JS (without using a wrapper around it and specifically targeting
.gatsby-background-image-[YOUR_ID] that is), as well as in media queries (my initial TODO)

and I don't like this behavior much ^^.

But maybe I'm just overthinking it oO?

EDIT: Clarification of style-concerns and addition of .gatsby-background-image-[YOUR_ID].

Aaah, I see, thanks! Actually I was pretty sure that I'm rewriting background-size property without !important flag, but it appears that I have a hack there 😃 I have two classes .background.animated which increase specificity.. 😅

So I'm okay with either of option 1 or option 2 and agree with @r007 regarding option 3. Actually his inline style proposal also looks nice.

timhagn · 2019-04-01T16:19:12Z

Hi @r007, @apaulau and all others : )!

Thanks for your thoughts and input, gonna revisit it tomorrow,
cause today my wife and I are celebrating our 12th relationship anniversary,
so no coding for me today ; )!

Have a nice day and read you tomorrow,

Tim.

petrpacas · 2019-04-01T17:06:20Z

Enjoy! 🎉

…

On 1 Apr 2019, 18:19 +0200, Tim Hagn ***@***.***>, wrote: Hi @r007, @apaulau and all others : )! Thanks for your thoughts and input, gonna revisit it tomorrow, cause today my wife and I are celebrating our 12th relationship anniversary, so no coding for me today ; )! Have a nice day and read you tomorrow, Tim. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

timhagn · 2019-04-02T09:11:17Z

@petrpacas Thanks, was a more than nice evening night - am still recuperating % ).

EDIT: And to everyone: no, it wasn't a joke - though we might just be the ultimate April Fool's One ^^.
From First of April, 2007 up till now we're together - married since Second of August 2008 (we didn't get an appointment at First of April '08 ; ).

timhagn · 2019-04-03T19:31:06Z

Hi all!

Recuperation took a little longer, so yesterday no more than merging in @davebra's PR #23, a few adaptions and publishing 0.2.8-beta (I really should get back to SemVer - there were just to many changes in transitions that needed tackling and testing in consort with you amazing people % ) did take place (except thanking everyone for the Congrats : ).

So on to what has changed and some more clarifying answers (& questions):

0.2.8-beta should be back on par with gatsby-image's transitions
After PR added id to the props of the component #23 an id is now directly present as a prop but pops the
First question: Should we just pass in all remaining props to the container element?
I'm already onto creating a function that transforms this.backgroundStyles back to
CSS kebab-case, which is also a preparation for being able to pass backgroundStyles from the style={{}} prop trough to the pseudo-elements (nice idea @r007, at the moment style only get's spread into the container element).
Second question: Should only background* or simply all styles be passed down to them?

To answer your above comment, @r007 (and add a question ; ):

Outer class .gatsby-background-image-[YOUR-ID] is an overkill, at least for me. Why would I needed it if I can simply assign class using className or id and then target it in my css.

I started to introduce .gatsby-background-image-[YOUR-ID] because, at least in my experience, the specificity of an such a wrapper class trumps some CSS-in-JS (primarily styled-components) direct targeting of the pseudo elements especially with the pre-parsed this.backgroundStyles values like background-position. Or did you think in the way of the
Third question: Should we be gone with .gatsby-background-image-[YOUR-ID] as well as the classId prop (and in this case, derive the pseudo-elements from the className prop given)?
I'd be more than wiling to engage in further discussion that's why here's the
Fourth and last question: Which should have higher specificity (get spread in / be parsed first): the backgroundStyles passed by the style={{}} prop or things passed and parsed via CSS(-in-JS)?

Thanks again and in advance,

Tim.

P.S.: @apaulau, could you please clarify your above comment and which specific hack you used (perhaps with some example code)?

Actually I was pretty sure that I'm rewriting background-size property without !important flag, but it appears that I have a hack there smiley I have two classes .background.animated which increase specificity..

EDIT: You can now see the above mentioned and slowly integrated changes in the branch:
0.2.8-kebabified.

timhagn · 2019-04-05T11:20:32Z

Latest version now is 0.2.8 and we're back on SemVer ^^.
Might anyone elaborate on my above comment, though, please : )?

petrpacas · 2019-04-05T11:40:52Z

There's just too much stuff to read/answer, man :-)

From my point of view:

It would make sense to just pass down {...props}
I don't think passing ALL styles to the pseudoElements is OK. I would stick with just bg related ones
I don't personally care, I won't use this feature anyway
It would make sense to prioritize inline styles - as it usually means higher specificity = as an override

timhagn · 2019-04-05T13:09:31Z

@petrpacas Thanks, these were nice and concise answers (yip, I tend to over-elaborate and do like chain-sentences, indeed ^^)!

With 2. and 4. you charge an open door with me, as those were my lines of thoughts as well.
With 1. I nearly agree, but am a little in doubt, if really any prop not already used and thus "filtered out" should be allowed (fearing a backlash of newly opened issues unrelated to this package ; ).
But I could let myself be convinced.
Might you elaborate a little more on your answer 3.? How are you styling a <BackgroundImage /> component at the moment?

Thanks again for your opinions! Hope the others chime in soon ; ).

apaulau · 2019-04-05T13:49:38Z

P.S.: @apaulau, could you please clarify your above comment and which specific hack you used (perhaps with some example code)?

Sure, here it is:

<BackgroundImage classNames="class1 class2" />

.class1::before,
.class1::after {...}

.class1.class2::before,
.class1.class2::after {
  background-size: 42% 42%;
}

Obviously .class1.class2 is more specific than .gatsby-background-image-[YOUR-ID] :)

timhagn · 2019-04-05T14:40:30Z

Thanks, @apaulau!
Didn't think about CSS-specificity (once again % )...
gatsby-background-image-[YOUR-ID] comes second to last in the <BackgroundImage />'s classList,
so your approach is more specific to the pseudo-elements (as much as I now gathered from reading the Specs again ; ).
Definitely speaks highly in favor for ... we [should] be gone with .gatsby-background-image-[YOUR-ID] as well as the classId prop (and in this case, derive the pseudo-elements from the className prop given) ^^.

timhagn · 2019-04-05T14:51:07Z

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

petrpacas · 2019-04-08T09:40:13Z

@timhagn
1/ TBH I can't imagine any issues 🤷‍♂️
3/ I'm using styled-components

timhagn · 2019-04-09T20:44:54Z

Thnx for your inputs!

The current state now is in the branch 0.2.9-kebabified, where all background-* from given className styles get spread into the pseudo-elements (for the moment without vendor-prefixes, see #22). Working on spreading in inline (background-) style={{}} as well.

I'm trying to deprecate the special .gatsby-background-image-[YOUR-ID] class and the
classId prop needed for it (without breaking the current behavior for the moment % ) as well
and just go with the given className(s) in v0.3.0 (which auto-magically should get media-queries working without !important even for styled-components and the like ; ).

How long do you think we should drag the old behavior with us (really until v1.0.0)?

itwasmattgregg · 2019-04-10T02:13:41Z

Till 1.0.0 is probably a good idea. Although I really wonder how many people took advantage of that. I found it easy enough to style with css modules so never felt I needed to target the id.

timhagn · 2019-04-10T14:40:32Z

@itwasmattgregg Thought about only taking them with us (pre-announced, of course) until 0.5.0 (yeah, I should have stuck to SemVer since the beginning ; ), or at least adding deprecation warnings right from the next version... Don't really know if anyone used it (except me in one small project, that is % ), but perhaps we'd get to know them in some issues after adding the warnings ^^.

timhagn · 2019-04-11T22:58:14Z

Now we are at 0.3.0 with most of your inputs realized! See the README for a How To and check gbitest for the newest implementation.
Please test as you wilth : )!

timhagn · 2019-04-15T11:45:22Z

Hi to all of you!

A little more has happened:

v0.3.1 brought additional prop passing to container - "safely" by excluding all propTypes of BackgroundImage from the remaining props (welcome ARIA and data-* attributes ; )
v0.3.2 was just a minor fix (forgot to include the title prop)
v0.3.3 (released just now) takes us mostly back in sync with gatsby-image IntersectionObserver handling (with additional "safety net" ; ) and adds some explanation on how to add a polyfill for it in IE & Safari

In 0.3.3 I also added the "new" fadeIn handling from gatsby-image (doesn't fade in if image is already in imageCache). Which do you prefer (the old 0.3.2 way or the synced one)?

Please test extensively : )! Should any of your tests reveal problems, just open an issue (or a PR, always welcome % )!

Should there be no hiccups on your sides, I'm gonna close this issue on Wednesday.

Fingers crossed,
best,
Tim.

petrpacas · 2019-04-15T12:00:42Z

The cached image not fading in is a funny thing - I noticed it w/ gatsby-image and it felt weird to have the cached images load straight up - as if something was broken w/ the plugin 😅

I'd say that it's best to keep the behaviour in sync w/ gatsby-image - most important thing is to behave consistently.

timhagn · 2019-04-15T12:08:49Z

@petrpacas Yup, seems a little strange - but I prefer synchronicity with gatsby-image for consistency reasons as well - though I changed the behavior a little bit on internal page change for _tracedSVG data, cause why the heck should the image trace in again (looks like flickering to me when you take a look at gbitest on the right side with gatsby-image and change between home and page-2 % )...

timhagn · 2019-04-16T18:42:43Z

v0.3.4 is out, which fixes "lost" changes on fluid / fixed image data (and tries to fade over).
Pertaining the fade in, it now makes it possible to set the fadeIn prop to 'soft' to emulate the old gatsby-image behavior (and ease the possibility of a crossfade ; ).

petrpacas · 2019-04-17T08:39:05Z

I wanted to suggest to add a prop for that, but you beat me to it - and I see gatsby-image has the fadeIn prop as well - did they just add it?

(https://www.gatsbyjs.org/packages/gatsby-image/#gatsby-image-props)

timhagn · 2019-04-17T11:26:15Z

@petrpacas The fadeIn prop of gatsby-image is a Boolean defaulting to true (and has been there since I started using Gatsby 2+ and digging deeper ; ). It can be overwritten by critical or set to false to directly show an image if it is in cache - kind of like their "new" behavior
(they added a new shouldFadeIn constant with this.state.fadeIn === true && !this.state.imgCached as a condition, which further controls the fading / not fading in of cached images).

As I thought it might be useful sometimes (have a case right now on image data change) to "force" a softer fade in, I just overloaded it with PropTypes.oneOfType([PropTypes.string, PropTypes.bool]) and simply added || this.props.fadeIn === 'soft' as a condition to shouldFadeIn.
It's still possible to overrule it with the critical prop, though % ).

timhagn · 2019-04-20T19:01:59Z

As no one else raised any flags and we're now at v0.3.6 I'm closing this one : ).

timhagn added enhancement New feature or request help wanted Extra attention is needed question Further information is requested labels Mar 27, 2019

This was referenced Mar 27, 2019

Add background-position property to background image #17

Closed

BG dissapears when 'seen' #16

Closed

timhagn mentioned this issue Apr 3, 2019

Deprecated props & excessive CSS rules #22

Closed

timhagn closed this as completed Apr 20, 2019

timhagn removed enhancement New feature or request help wanted Extra attention is needed question Further information is requested labels Jul 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`background-*` passthrough or singular pseudo-Element styling? #20

`background-*` passthrough or singular pseudo-Element styling? #20

timhagn commented Mar 27, 2019 •

edited

petrpacas commented Mar 28, 2019

timhagn commented Mar 28, 2019

timhagn commented Mar 28, 2019

apaulau commented Mar 29, 2019

timhagn commented Mar 29, 2019 •

edited

r007 commented Mar 31, 2019

timhagn commented Apr 1, 2019 •

edited

r007 commented Apr 1, 2019 •

edited

apaulau commented Apr 1, 2019

timhagn commented Apr 1, 2019

petrpacas commented Apr 1, 2019 via email

timhagn commented Apr 2, 2019 •

edited

timhagn commented Apr 3, 2019 •

edited

timhagn commented Apr 5, 2019

petrpacas commented Apr 5, 2019 •

edited

timhagn commented Apr 5, 2019

apaulau commented Apr 5, 2019

timhagn commented Apr 5, 2019

timhagn commented Apr 5, 2019

petrpacas commented Apr 8, 2019

timhagn commented Apr 9, 2019

itwasmattgregg commented Apr 10, 2019

timhagn commented Apr 10, 2019

timhagn commented Apr 11, 2019

timhagn commented Apr 15, 2019

petrpacas commented Apr 15, 2019

timhagn commented Apr 15, 2019

timhagn commented Apr 16, 2019

petrpacas commented Apr 17, 2019

timhagn commented Apr 17, 2019

timhagn commented Apr 20, 2019

background-* passthrough or singular pseudo-Element styling? #20

background-* passthrough or singular pseudo-Element styling? #20

Comments

timhagn commented Mar 27, 2019 • edited

petrpacas commented Mar 28, 2019

timhagn commented Mar 28, 2019

timhagn commented Mar 28, 2019

apaulau commented Mar 29, 2019

timhagn commented Mar 29, 2019 • edited

r007 commented Mar 31, 2019

timhagn commented Apr 1, 2019 • edited

r007 commented Apr 1, 2019 • edited

apaulau commented Apr 1, 2019

timhagn commented Apr 1, 2019

petrpacas commented Apr 1, 2019 via email

timhagn commented Apr 2, 2019 • edited

timhagn commented Apr 3, 2019 • edited

timhagn commented Apr 5, 2019

petrpacas commented Apr 5, 2019 • edited

timhagn commented Apr 5, 2019

apaulau commented Apr 5, 2019

timhagn commented Apr 5, 2019

timhagn commented Apr 5, 2019

petrpacas commented Apr 8, 2019

timhagn commented Apr 9, 2019

itwasmattgregg commented Apr 10, 2019

timhagn commented Apr 10, 2019

timhagn commented Apr 11, 2019

timhagn commented Apr 15, 2019

petrpacas commented Apr 15, 2019

timhagn commented Apr 15, 2019

timhagn commented Apr 16, 2019

petrpacas commented Apr 17, 2019

timhagn commented Apr 17, 2019

timhagn commented Apr 20, 2019

`background-*` passthrough or singular pseudo-Element styling? #20

`background-*` passthrough or singular pseudo-Element styling? #20

timhagn commented Mar 27, 2019 •

edited

timhagn commented Mar 29, 2019 •

edited

timhagn commented Apr 1, 2019 •

edited

r007 commented Apr 1, 2019 •

edited

timhagn commented Apr 2, 2019 •

edited

timhagn commented Apr 3, 2019 •

edited

petrpacas commented Apr 5, 2019 •

edited