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

BG dissapears when 'seen' #16

Closed
petrpacas opened this issue Mar 19, 2019 · 42 comments
Closed

BG dissapears when 'seen' #16

petrpacas opened this issue Mar 19, 2019 · 42 comments
Labels
bug Something isn't working

Comments

@petrpacas
Copy link

Hey Tim,

after your recent changes, after you run across an already 'seen' bg-image (which also gets logged to the console now), the image dissapears.

Also, not sure if the opacity: 0.99; of the parent element is okay for the child elements of such container?

@timhagn
Copy link
Owner

timhagn commented Mar 19, 2019

Hi Petr,

ah, now I understand what @adam-wheatley meant at his self closed PR #15 % ).
Might be because the imageCache constant isn't exported from ImageUtils.js
Already wanted to look into it (and maybe use a Hook therefore ; ) - though rather
cause I ran into "cache-misses" instead of disappearing images - the (forgotten)
console.log() in handleImageLoaded() didn't fire at my side.
Gonna try to dig into it today.

On the other hand, the opacity: 0.99 on the parent element was necessary as of issue #8;
it here just creates a new stacking context for the children. Did you notice any problems with
child elements on your side?

Thanks for the clarification to #15 (was afk till today % )!

Best,

Tim.

@timhagn timhagn mentioned this issue Mar 19, 2019
Closed
@timhagn
Copy link
Owner

timhagn commented Mar 19, 2019

@petrpacas & @adam-wheatley, just published version 0.2.73 which should solve the issue (at least it does so in gbitest when changing pages).
Had a Typo in ImageUtils.js on line 73, had written img.src = imageData.src ? imageData.srcSet : ''
instead of img.src = imageData.src ? imageData.src : ''
(of course the '' are back-ticks in the file itself ; ).

OK - closing this issue wasn't my intention, the commit msg of 13470cb did do this by the wording (learning something new about GitHub every other day ; ).
Of course the honors to close are your's, Petr, as long as the new version really fixes the issue : ).

@timhagn timhagn reopened this Mar 19, 2019
@petrpacas
Copy link
Author

petrpacas commented Mar 19, 2019

I'm afraid the issue persists for me 😢 - after switching pages back and forth the background-image is missing in the ::before. It's present in the ::after, but background-color of ::before is overlaying it.

@adam-wheatley
Copy link

Yup, I gave it a go aswell and problem is still there

@timhagn
Copy link
Owner

timhagn commented Mar 19, 2019

OK, gonna sleep over it and try to dig into it tomorrow. Chromium and FF on Linux Mint 19.1 seemed to work, but alas : /.

@timhagn timhagn added bug Something isn't working help wanted Extra attention is needed labels Mar 21, 2019
@timhagn
Copy link
Owner

timhagn commented Mar 21, 2019

Hey there, sorry to say, won't come around to it till the weekend as of personal matters : /.
A workaround I gathered from your comments and my (short) experiments might be, to set
background-color of ::before to transparent !important for the moment.

Added Tags to this issue if someone would want to take on this before me...
Edit: added !important

@petrpacas
Copy link
Author

Hey, no worries Tim - I just went back to an older version for the time being

@timhagn
Copy link
Owner

timhagn commented Mar 21, 2019

@petrpacas, replies as your's still keep me believing in OSS-Culture : ). Thanks therefore!
Might I ask, which latest older version still works for you?

@petrpacas
Copy link
Author

Cheers!

I reverted to 0.2.61 - but only because I upgraded from that version - not sure where the issue started

@timhagn
Copy link
Owner

timhagn commented Mar 22, 2019

Thanks : )!
0.2.61 should be an acceptable choice - as long as you don't encounter issue #12 (then 0.2.62 would solve it) or want to use the additional _noBase64 in your GraphQL queries.
Cause methinks the problem might have started after "solving" issue #13, pertaining the latter (therefore I rewrote the transitions). So probably 0.2.72 started this issue % |.
Gonna get back at it as soon as I can (or someone adds a PR solving it ; ).

@timhagn
Copy link
Owner

timhagn commented Mar 25, 2019

Is the problem still existing after the merge of PR #18? At least here I can't reproduce it for the moment anymore oO... Now gbi is at 0.2.74.

@petrpacas
Copy link
Author

Unfortunately, even with clean yarn/gatsby caches, this bug is still there for me...

@timhagn
Copy link
Owner

timhagn commented Mar 25, 2019

*miff* My weekend wasn't that great, so I found no time to dig into it. But I'm gonna keep it in mind.
Might you (once more) give me any info about your System / Browser, so I can replicate it (in a VM if needs be)? gatsby info plus direct usage (example code) might be of help. Here I couldn't replicate it with or without background color : /.

Edit: Though I could replicate the :before having opacity: 1 afterwards, don't get me wrong % |

@petrpacas
Copy link
Author

petrpacas commented Mar 25, 2019

MacOS/Chrome here...

gatsby info

  System:
    OS: macOS 10.14.3
    CPU: (4) x64 Intel(R) Core(TM) i5-4250U CPU @ 1.30GHz
    Shell: 5.0.3 - /usr/local/bin/bash
  Binaries:
    Node: 11.12.0 - ~/.nvm/versions/node/v11.12.0/bin/node
    Yarn: 1.15.2 - /usr/local/bin/yarn
    npm: 6.9.0 - ~/.nvm/versions/node/v11.12.0/bin/npm
  Languages:
    Python: 2.7.16 - /usr/local/bin/python
  Browsers:
    Chrome: 72.0.3626.121
    Firefox: 61.0.2
    Safari: 12.0.3
  npmPackages:
    gatsby: ^2.2.2 => 2.2.2 
    gatsby-background-image: 0.2.74 => 0.2.74 
    gatsby-image: ^2.0.34 => 2.0.34 
    gatsby-plugin-manifest: ^2.0.24 => 2.0.24 
    gatsby-plugin-netlify-cache: ^1.2.0-beta.0 => 1.2.0-beta.0 
    gatsby-plugin-offline: ^2.0.25 => 2.0.25 
    gatsby-plugin-prefetch-google-fonts: ^1.4.0 => 1.4.0 
    gatsby-plugin-react-helmet: ^3.0.10 => 3.0.10 
    gatsby-plugin-sharp: ^2.0.29 => 2.0.29 
    gatsby-plugin-sitemap: ^2.0.10 => 2.0.10 
    gatsby-plugin-styled-components: ^3.0.7 => 3.0.7 
    gatsby-source-filesystem: ^2.0.27 => 2.0.27 
    gatsby-transformer-sharp: ^2.1.17 => 2.1.17 
  npmGlobalPackages:
    gatsby-cli: 2.4.17

As for the example - I tried to extract the relevant bits here:

import React from 'react'
import styled from 'styled-components'
import BackgroundImage from 'gatsby-background-image'

const Background = styled(BackgroundImage)`
  align-items: center;
  background-position: center
    ${props => (props.isHomepage ? 'bottom' : 'center')};
  display: flex;
  flex-direction: column;
  justify-content: center;
  margin-bottom: 3rem;
  margin-top: 4.5rem;
  min-height: ${props => props.isHomepage && 'calc(100vh - 4.5rem)'};
  padding: ${props => !props.isHomepage && '9rem 0'};
  text-align: center;

  &:before,
  &:after {
    background-position: center
      ${props => (props.isHomepage ? 'bottom' : 'center')};
  }
`

export default ({ coverImage, isHomepage, title, subtitle }) => (
  <Background
    backgroundColor={`gray`}
    fluid={coverImage}
    isHomepage={isHomepage}
    Tag="section"
  />
)

...
I'm passing the coverImage={data.cover.childImageSharp.fluid} as a prop
page.js >> layout.js >> coverImage.js
...

  query {
    cover: file(relativePath: { eq: "example.jpg" }) {
      childImageSharp {
        fluid(maxWidth: 2048, quality: 75) {
          ...GatsbyImageSharpFluid_withWebp
        }
      }
    }
  }

@milanito
Copy link

Hello

I wonder if a combination of styled-component with HMR is not the issue here. I have had problems with it.

Does it have the same behavior using BackgrounImage directly ?

@petrpacas
Copy link
Author

As soon as I posted it I thought as much as well - sadly, no - same behaviour even when using straight-up BGI

@petrpacas
Copy link
Author

Hmm... What would solve this would be to just remove <Background backgroundColor={`gray`} bit...

But I believe this shouldn't be needed, correct?

@timhagn
Copy link
Owner

timhagn commented Mar 25, 2019

Thanks for your efforts, more than appreciated!
Methinks, there might to have to be introduced a minimal "state machine" for the pseudo-elements - and the fallback (with no JS) should be a simple background-image on the main element instead of the one taken from gatsby-image's noscript picture element. This one has definitely outgrown the "pilfering" from the original - and I never would have thought it might get this "popular" oO...

Edit: wrong brackets in the sentences.

@petrpacas
Copy link
Author

I haven't checked the code, but I was very surprised that somebody managed to convert the original image plugin into a background-image one!

Well, you made a plugin that is filling a very important feature gap for Gatsby, so there you go 💪🎉

@timhagn
Copy link
Owner

timhagn commented Mar 26, 2019

Aww, thanks, that's nice of you to say : )!
And it all just started when I needed a stretchable background for a @freeCodeCamp Project
(went through their entire curriculum last year during the #100DaysOfCode challenge for a refreshment and the Certs ; ).

@petrpacas
Copy link
Author

Nice, I love FCC!

timhagn added a commit that referenced this issue Mar 26, 2019
timhagn added a commit that referenced this issue Mar 26, 2019
@timhagn
Copy link
Owner

timhagn commented Mar 26, 2019

Now, after digging into the problem it seems to be solved for the moment - at least when I tested it
with newest Chrome, FF, and Safari (12.1) on a macOS 10.12.1 (Sierra) VM (which just updates to Mojave) as well as on a Windows 10 VM with Chrome, FF, Edge & IE 11 ^^ (and of course Chromium & FF on Mint 19.1).
Published Version 0.2.8-alpha1 - would you be so kind to test it at your place, @petrpacas?

Cause it seemed to run, but Chrome brought the macOS VM down to a crawl (no problems in FF & Safari) and having both VMs (with 8GB and 2 cores each) running, while having open 3 IntelliJ Idea and some browser windows brought even my dev-rig (i7-9700K with 32GB of RAM) nearly down to unresponsiveness % ).

Have (definitely!) still work TODO on the transitions, but your initial issue seems fixed for the moment
(I do hope so at least ; ).

@petrpacas
Copy link
Author

petrpacas commented Mar 26, 2019

Indeed, the BG doesn't hide anymore - good job! 👏

Haha, I have basic 2013 Macbook Air, I could never run that kind of op 😅 (Btw, not sure if applicable for you, but give VS Code a go 👍)

(Just a sidenote, I'm getting this warning duruing gatsby bootstrap - didn't get it before, not sure if related: )

success extract queries from components — 0.334 s
⠁ (node:1540) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 drain listeners added. Use emitter.setMaxListeners() to increase limit
success run graphql queries — 1.259 s — 19/19 15.12 queries/second

@timhagn
Copy link
Owner

timhagn commented Mar 26, 2019

I used Code on the VMs to test it with gbitest ^^. (Just love Idea for bigger projects ; ).

Will look into the other issue, though I didn't get the MaxListenersExceededWarning on either VM Guest or my Host machine (with aforementioned example repo) neither with gatsby develop nor gatsby build oO... But more tweaking tomorrow, started at ~13h and just finished for the day ^^.

@petrpacas
Copy link
Author

petrpacas commented Mar 27, 2019

All your work is very appreciated! Don't stress about the warning tho, most likely something on my end...

EDIT: Indeed, the warning is now present even for older versions of GBI - so - it's unrelated...

EDIT2: Okay - I get the warning because I have too many single-file queries on a certain page. I could do an allFile query instead, but afaik I'd have to reference images by edges[idx] - that seems very cumbersome...

@timhagn
Copy link
Owner

timhagn commented Mar 27, 2019

Thanks a lot for the compliment (I'm bad at receiving them ; )!
Great, that you could detect the cause of the MaxListenersExceededWarning yourself,
good to know, gonna remember the single-file queries for future projects - there I wonder, if splitting the query in single ones might help.
Or you could use a transform function on the edges array before further usage within your project, spitting out an object with the names of the files as uppermost keys or suchlike
(at least that's what I'd probably do ; ).

EDIT: Forgot a 'the'. By the way we're at 0.2.8-alpha3 now with #17 merged in and #20 up for question.

@petrpacas
Copy link
Author

Haha, I'm bad at that as well, no worries 🙌

Transform function... will have to read that up 🤔

@timhagn
Copy link
Owner

timhagn commented Mar 28, 2019

Don't know if "transform function" is an official term, just describes what it does. You can look one up in my "Portfolio Page" project for fCC here, though it uses map() with a project-pager (how many projects shall be shown) beforehand. Depending on your case and how you want the returned AllFiles edges used (per my example the filenames as a key) I'd perhaps rather go with a reduce() over the edges array returning an object. Just ask should you need any help : ).

@timhagn
Copy link
Owner

timhagn commented Mar 28, 2019

@petrpacas Now we're at 0.2.8-alpha4 - might you confirm that the transitions are working again
"on par" (or as near as one might get) with gatsby-image, so we might finally close this issue?

@petrpacas
Copy link
Author

petrpacas commented Mar 29, 2019

Wasn't aware my issue was related to transitions, but yeah, I did notice the stopped being nice at some point...

Well, they do look very nice now 😉

PS: Okay, I thought there was some "transform function" in GraphQL, I see what you mean now. Thanks for the suggestion 🙏

EDIT: I just got around to reading about the stacking context - that opacity: 0.99; hack is still needed, yes? Funny thing 🤔

@timhagn
Copy link
Owner

timhagn commented Mar 29, 2019

Yeah, had everything to do with transitions (or better: my implementation of them on state change), same with issues #10 and #13. Opposed to gatsby-image with several internal placeholder-images, we have only two pseudo-elements to let "the magic" happen - and as we were one step short when an image all already seen, ::before came to be opacity: 1; in the end, although it should have been 0, so ::after (with then in "possession" of the final image to be shown) is up front.

Rewrote my rewrites for the _noBase64 issue and we are where we are now, with the transitions back on track (tested it with all default GraphQL Fragments) ^^.

The opacity hack still seems to be needed, indeed - am not completely happy with it myself, but the alternatives seemed worse in context of stacking (context)...

And you're welcome, of course, digging out a link to my own repo's file wasn't that hard a work to do ; ).
Btw.: In the getBackgroundStylesForSingleClass() function found in BackgroundUtils,js line 108 there's this little piece of a reducer returning an object, which you could use as well on the edges array.

There are schema transforms in GraphQL, but as the name says, they'd have to be applied after schema creations and thus on the backend side of Gatsby - methinks the above is easier ; ).

@itwasmattgregg
Copy link

itwasmattgregg commented Apr 2, 2019

Not sure if this is exactly the same issue but I'm on Windows Chrome with the 0.2.8-alpha4 patch and it's not working. When navigating back to a page with bg image you've already been on there is no background-image property in the :before tag although there is one in the :after tag. Just thought I would report some more on this issue.

Also checked on mac chrome. Was ok with an older version on mac.

@timhagn
Copy link
Owner

timhagn commented Apr 2, 2019

Hi @itwasmattgregg,

might I guess you meant 0.2.8-alpha4? Cause I just tested it in a Win10 VM with Chrome (Version 73.0.3683.86 (Official Build) (64-bit)) and can't replicate your issue (tested all possible [default] GraphQL Fragments). Would you be so kind and give me a short gatsby info?

Mac Chrome (in a macOS Mojave VM ; ) is most of times to slow for a real "testing" experience,
so thanks for checking : ).

@itwasmattgregg
Copy link

Yea I meant that alpha4 sorry. I'll test on my windows machine when I get home this evening. Thanks for looking into it. I'll check my Chrome version as well.

@timhagn
Copy link
Owner

timhagn commented Apr 2, 2019

master is now again the current branch with today's release of 0.2.8-beta (after merging in #23).
Hope that works for you @itwasmattgregg - I'm intrigued what your tests will bring : )!

@timhagn
Copy link
Owner

timhagn commented Apr 3, 2019

Hi @itwasmattgregg!

Did you already get the chance to test 0.2.8-beta on your windows machine so we might close this issue - or is anything still amiss?

Thanks in advance,

Tim.

@itwasmattgregg
Copy link

It works :) Honestly something most likely happened with the yarn install that didn't actually install the right package when I was on the windows machine. It all works fine now with 0.2.8-beta. Thanks!

@timhagn
Copy link
Owner

timhagn commented Apr 5, 2019

Great, @itwasmattgregg : )! Sometimes one just needs to "use the force" (yarn --force that is ; ).
Thanks for testing it (although giving me a small fright in the first place ^^), anyways!
And if you want, you're more than welcome to head over to issue #20 to raise your opinion on the (near) future progress of this package!

And @petrpacas: Would you wanna have the honors of closing this issue, or shall I?

@petrpacas
Copy link
Author

Thank you, good sir 🤙

@timhagn
Copy link
Owner

timhagn commented Apr 5, 2019

@petrpacas Wow, that was fast : ). And you're welcome, of course! I always rather like (hopefully satisfied ; ) authors to close their issues themselves, would feel rude to me if I just did it ^^.
Am just polishing CONTRIBUTING.md a little and then we're back on SemVer with 0.2.8 ; ).
Btw. would you perhaps have an opinion on how I should recognize every contributor (which also should include people like you and any other who "just" raised an issue pertaining bugs or reviews)?
Tried all-contributors yesterday, but that feels a little over the top...

@petrpacas
Copy link
Author

petrpacas commented Apr 5, 2019

It's kind of you to want to do that. But indeed that's bit of an overkill IMO :-)

I'm not a big OSS contributor, so I don't know if it's a common practice to mention them 🤷‍♂️

@timhagn
Copy link
Owner

timhagn commented Apr 5, 2019

Methinks it's common in bigger projects, but why not "lead by example" for smaller ones ; )?
And hey, you contributed quite some time to this open source one already, don't sell yourself short : )!

Added the question to the CONTRIBUTING file, so just let's wait, what others have to say.
Anyone out there gotta want some recognition, I bet ^^.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants