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

Degrade gracefully when external JS can’t be loaded #248

Merged
merged 1 commit into from
Sep 12, 2016

Conversation

robinwhittleton
Copy link
Contributor

@robinwhittleton robinwhittleton commented Sep 8, 2016

There are plenty of situations where external JS could fail to load and be unavailable. For performance reasons we apply the js-enabled class inline, which can lead to a mismatch between the JS availability and the styling hook.

To work around this, we can check at the end of the document to see whether GOVUK (our standard module namespace) exists. If not then the external JS hasn’t triggered: at this point we can remove the js-enabled class from the body and fall back to the non-JS styling.

@robinwhittleton
Copy link
Contributor Author

I would love some discussion around this before it’s merged.

@tombye
Copy link
Contributor

tombye commented Sep 9, 2016

It seems a bit odd to use the js-enabled class to indicate the presence of window.GOVUK only as the words imply something else.

It also seems to assume all external JS that attaches to window.GOVUK will load in one file so if window.GOVUK exists they all do. Not sure it works if you're serving them from more than one file and not all load (for example if nodes of your CDN don't all update correctly so certain locations point to files the CDN doesn't possess).

Personally, my problem is with using js-enabled rather than this change. Unless I'm wrong, all js-enabled tells you is scripting is enabled on the client rather than certain scripts are running. This is the reason I'm sceptical of using it because that information doesn't help much. I much prefer your approach of each module adding classes to indicate it is present and running (in alphagov/govuk_frontend_toolkit#317) because I can declare styles specific to the module which are only activated when its JS runs.

@robinwhittleton
Copy link
Contributor Author

robinwhittleton commented Sep 9, 2016

Leaving aside the radio buttons work, this attempts to fix a more general situation. There will be exceptions yes (your single file from multiple failing is one) but it certainly resolves the broken connection to the CDN issue.

Regarding adding classes in the external JS, that swaps out that problem of potential broken connections for a delay in applying JS-specific styling. This can lead to a flash of unstyled content, for example panels that are meant to be hidden on page load showing for the time it takes to load, parse and run the JS. Either ways has pros and cons.

@tombye
Copy link
Contributor

tombye commented Sep 9, 2016

Looking at the alphagov repos is helpful for an idea of usage:

https://github.com/search?l=&p=1&q=js-enabled+user%3Aalphagov+language%3ASCSS&ref=advsearch&type=Code&utf8=%E2%9C%93

From that it looks like we do use js-enabled as a straight boolean in the CSS targeting a state when JS for the thing being styled is both available and runs. I also can't find anything there styling JS behaviours not part of the window.GOVUK namespace.

In other words, the way it's being used by us gives a pretty strong reason for this change as it would benefit the majority.

Conscious that it would be helpful if someone other than me to comment on this too :)

There are plenty of situations where external JS could fail to load and be unavailable. For performance reasons we apply the js-enabled class inline, which can lead to a mismatch between the JS availability and the styling hook.

To work around this, we can check at the end of the document to see whether GOVUK (our standard module namespace) exists. If not then the external JS hasn’t triggered: at this point we can remove the js-enabled class from the body and fall back to the non-JS styling.
@NickColley
Copy link
Contributor

LGTM 👍

@gemmaleigh gemmaleigh merged commit 94c0f7f into master Sep 12, 2016
robinwhittleton pushed a commit that referenced this pull request Sep 12, 2016
- Degrade gracefully when external JS can’t be loaded (PR #248)
gemmaleigh added a commit to alphagov/govuk_elements that referenced this pull request Sep 12, 2016
gemmaleigh added a commit to alphagov/govuk-prototype-kit that referenced this pull request Sep 12, 2016
# 0.18.2

- Degrade gracefully when external JS can’t be loaded ([PR
#248](alphagov/govuk_template#248))
@robinwhittleton robinwhittleton deleted the ensure_js_status branch September 12, 2016 10:37
Guntrisoft added a commit to guidance-guarantee-programme/pension_guidance that referenced this pull request Dec 2, 2016
Makes the following changes:

Remove generated gov.uk from relative print links
alphagov/govuk_template#234

Fix extended footer on certain pages
alphagov/govuk_template#177

Degrade gracefully when external JS can’t be loaded
alphagov/govuk_template#248

Add docs for adding tabindex="-1" to fix the skiplink
alphagov/govuk_template#250

Logo fixes
alphagov/govuk_template#237

Remove external links styles
alphagov/govuk_template#231

Don’t include both html5shiv and html5shiv-printshiv
alphagov/govuk_template#254

Update govuk_frontend_toolkit to 5.0.0
alphagov/govuk_template#256

Fixed scala compilation failure for play template
alphagov/govuk_template#261
Guntrisoft added a commit to guidance-guarantee-programme/pension_guidance that referenced this pull request Dec 2, 2016
Makes the following changes:

Remove generated gov.uk from relative print links
alphagov/govuk_template#234

Fix extended footer on certain pages
alphagov/govuk_template#177

Degrade gracefully when external JS can’t be loaded
alphagov/govuk_template#248

Add docs for adding tabindex="-1" to fix the skiplink
alphagov/govuk_template#250

Logo fixes
alphagov/govuk_template#237

Remove external links styles
alphagov/govuk_template#231

Don’t include both html5shiv and html5shiv-printshiv
alphagov/govuk_template#254

Update govuk_frontend_toolkit to 5.0.0
alphagov/govuk_template#256

Fixed scala compilation failure for play template
alphagov/govuk_template#261
NickColley added a commit to alphagov/govuk-frontend that referenced this pull request May 29, 2018
Changes from [govuk_template](https://github.com/alphagov/govuk_template):

Breaking changes:
- Removed stylesheets relating to `govuk_template.css`, we now recommend people bundle stylesheets in their application.
- Removed stylesheets relating to `print`, we do this at a component level.
- Removed stylesheets relating to `font`, we now have a [new font loading strategy](#726) that does not require a different bundle.
- Removed `ie8` specific class being added to the `html` element, since we don't require functionality
- Remove `ie.js`, used to shim HTML5 files, add JSON2 support, we now recommend people bundle in their application.

- Removed functionality to remove `js-enabled` class if `window.GOVUK` is not defined, at the moment we can't guarantee that this global will be set with GOV.UK Frontend.
See alphagov/govuk_template#248 for details on this feature.

Features:
- Added a main block, now skip link works by default, this block can be overridden.

Patch:
- Moved Skip link into it's own block, calling the [Skip link component](https://github.com/alphagov/govuk-frontend/tree/master/src/components/skip-link) by default instead of being inlined.
- Moved footer into it's own block, calling the [Footer component](https://github.com/alphagov/govuk-frontend/tree/master/src/components/footer) by default instead of being inlined.
- Moved header into it's own block, calling the [Header component](https://github.com/alphagov/govuk-frontend/tree/master/src/components/header) by default instead of being inlined.
- Changed the default `main` `id` to be `#main-content` to avoid being [styled by GOV.UK Elements](https://github.com/alphagov/govuk_elements/blob/8216538c1d2efb8b33372a22f28e1ea81d879ecd/assets/sass/elements/_layout.scss#L9).
NickColley added a commit to alphagov/govuk-frontend that referenced this pull request May 31, 2018
Changes from [govuk_template](https://github.com/alphagov/govuk_template):

Breaking changes:
- Removed stylesheets relating to `govuk_template.css`, we now recommend people bundle stylesheets in their application.
- Removed stylesheets relating to `print`, we do this at a component level.
- Removed stylesheets relating to `font`, we now have a [new font loading strategy](#726) that does not require a different bundle.
- Removed `ie8` specific class being added to the `html` element, since we don't require functionality
- Remove `ie.js`, used to shim HTML5 files, add JSON2 support, we now recommend people bundle in their application.

- Removed functionality to remove `js-enabled` class if `window.GOVUK` is not defined, at the moment we can't guarantee that this global will be set with GOV.UK Frontend.
See alphagov/govuk_template#248 for details on this feature.

Features:
- Added a main block, now skip link works by default, this block can be overridden.

Patch:
- Moved Skip link into it's own block, calling the [Skip link component](https://github.com/alphagov/govuk-frontend/tree/master/src/components/skip-link) by default instead of being inlined.
- Moved footer into it's own block, calling the [Footer component](https://github.com/alphagov/govuk-frontend/tree/master/src/components/footer) by default instead of being inlined.
- Moved header into it's own block, calling the [Header component](https://github.com/alphagov/govuk-frontend/tree/master/src/components/header) by default instead of being inlined.
- Changed the default `main` `id` to be `#main-content` to avoid being [styled by GOV.UK Elements](https://github.com/alphagov/govuk_elements/blob/8216538c1d2efb8b33372a22f28e1ea81d879ecd/assets/sass/elements/_layout.scss#L9).
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.

4 participants