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

fix(background): set bing background before lazyload initialization #393

Merged
merged 1 commit into from
Jul 24, 2017

Conversation

koyabr
Copy link
Contributor

@koyabr koyabr commented Jun 15, 2017

What kind of change does this PR introduce? (check one with "x")

  • Bug fix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

Does this PR introduce a breaking change? (check one with "x")

  • Yes
  • No

Description

In my chrome, the bing background fails to be loaded. After I swap the two js code blocks (1. set the data-original attr of body 2. initialize lazyload), the bing picture is successfully fetched.

I'm no expert in javascript, my best guess is that the order in which codes are written is important for lsloader.runInlineScript().


Verification steps

No verification steps.

@neoFelhz neoFelhz self-requested a review June 15, 2017 08:53
Copy link
Collaborator

@neoFelhz neoFelhz left a comment

Choose a reason for hiding this comment

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

We are using config_css.ejs to config theme.background.bgimg, and using lazyload with queue in import_js.ejs to load bing background, as bing background with aws cdn would cause terrible performance when loading.

@koyabr koyabr changed the title fix(background): set background image/color according to config fix(background): set bing background before lazyload initialization Jun 15, 2017
@koyabr
Copy link
Contributor Author

koyabr commented Jun 15, 2017

@neoFelhz the lazyload idea is nice, I looked around in import_js.ejs and found a small tweak which works for me, please review my update, thanks.

@neoFelhz neoFelhz requested review from cubesky and iblh June 15, 2017 12:13
@neoFelhz neoFelhz dismissed their stale review June 15, 2017 12:14

We will discuss about it.

@neoFelhz neoFelhz closed this Jun 24, 2017
@neoFelhz neoFelhz reopened this Jun 24, 2017
@neoFelhz neoFelhz merged commit 9ab4ea2 into iblh:canary Jul 24, 2017
This was referenced Jul 24, 2017
@koyabr koyabr deleted the fix/background branch November 11, 2017 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants