-
Notifications
You must be signed in to change notification settings - Fork 15
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
Reorder HEAD elements so that CSS comes before JavaScript #245
base: master
Are you sure you want to change the base?
Conversation
@maethu, I looked at it but don't feel confident to review that. Could you please take a look at it? |
I second that. He pushes all scripts beneath the CSS and not only the one that isn't working. This might have other implications though it would be (as he said) the right order. But maybe this is a thing that we want to fix in colorbox? (javascript onload) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question:
Would it be enough to just swap line 52 and 54? I'm not sure about the impact of the other changes.
docs/HISTORY.txt
Outdated
@@ -4,7 +4,8 @@ Changelog | |||
4.0.6 (unreleased) | |||
------------------ | |||
|
|||
- Nothing changed yet. | |||
- Reorder HEAD elements so that CSS comes before JavaScript - which is correct and proper | |||
This solves an issue in ftw.colorbox where overlays are not always loaded. [djowett-ftw] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@djowett-ftw Since I installed the change already on the DEV System, this is not true. The first line is Reorder HEAD elements so that CSS comes before JavaScript - which is correct and proper
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question:
Would it be enough to just swap line 52 and 54? I'm not sure about the impact of the other changes.
There's just one CSS and no JS in #portal-top for izug, so no that wouldn't be enough.
The extraction (and re-ordering) of the new line 47 from 41 is the main bit, but it's proper to put #portal-top script after that too (and that's a no-op for izug I believe)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@djowett-ftw Since I installed the change already on the DEV System, this is not true. The first line is
Reorder HEAD elements so that CSS comes before JavaScript - which is correct and proper
Good spot! Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question:
Would it be enough to just swap line 52 and 54? I'm not sure about the impact of the other changes.There's just one CSS and no JS in #portal-top for izug, so no that wouldn't be enough.
The extraction (and re-ordering) of the new line 47 from 41 is the main bit, but it's proper to put #portal-top script after that too (and that's a no-op for izug I believe)
@maethu was my argument convincing?
…correct and proper This solves an issue in ftw.colorbox where overlays are not always loaded
4f92ecb
to
2d22586
Compare
This is correct and proper and
it solves an issue in ftw.colorbox where overlays are not always loadedis in line with colorbox requirements