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

Detect whether Google Sheets will work #491

Merged
merged 33 commits into from
Nov 19, 2019
Merged

Detect whether Google Sheets will work #491

merged 33 commits into from
Nov 19, 2019

Conversation

tunetheweb
Copy link
Member

@tunetheweb tunetheweb commented Nov 14, 2019

Makes progress on #432
Makes progress on #428
Makes progress on #413 (comment)
Makes progress on #379

I present this PR for your consideration and for discussion.

Basically what I did was:

  • Remove the iframes. The img tags load by default and will be progressively enhanced with Sheets, which are added later with JavaScript if supported.
  • Add data- elements to the img tag with the information needed to recreate the iframe.
  • Added a js/chapter.js script which a couple of checks and then enables Sheets visuals if they pass. Quite a bit of JavaScript so decided to pull it out into a separate sheet rather than inline it. Edit: also moved HTTP Archive discussion counter check into that too.
  • This has added benefit that those that can't use them, don't wastefully load Sheets as discussed on Audit the website for performance issues #413 (comment)
  • Desktop will still load img's and then iframe's but I can live with that for now.
  • Add a test pixel for Google Docs favicon with an error function to revert the Sheets back to Images when Google Docs is blocked (e.g. in some corporate environments).
  • Added an alt tag to imgs, and a title tags to iframes and changed aria-labelledby to point to figure (to avoid duplicating alt text to assistive technologies) and also added an aria-describedby to point to more descriptive text, as discussed in Images missing alt text #379 (comment)

The HTTP/2 and Accessibility chapters are the only ones on new format as part of this PR. Will convert others with a follow up PR if this is approved. It's backwards compatible but thought I'd get your thoughts before doing the other chapters in a similar manner.

This seems to work on iPads now AND in my work which blocks Google Sheets.

src/static/js/chapter.js Show resolved Hide resolved
src/static/js/chapter.js Outdated Show resolved Hide resolved
src/static/js/chapter.js Outdated Show resolved Hide resolved
src/static/js/chapter.js Outdated Show resolved Hide resolved
src/templates/en/2019/base_chapter.html Outdated Show resolved Hide resolved
@rviscomi rviscomi added the development Building the Almanac tech stack label Nov 15, 2019
@rviscomi rviscomi added this to the SHIP IT! milestone Nov 15, 2019
Copy link
Member

@rviscomi rviscomi left a comment

Choose a reason for hiding this comment

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

LGTM just a couple of small comments. Hoping this solves our data viz issues!

src/static/js/chapter.js Show resolved Hide resolved
src/static/js/chapter.js Outdated Show resolved Hide resolved
@tunetheweb
Copy link
Member Author

BTW once we're happy it would be good to ship this to prod, so can test the HTTP/2 chapter again in live before applying the similar changes to the other chapters.

src/static/js/chapter.js Outdated Show resolved Hide resolved
src/static/js/chapter.js Outdated Show resolved Hide resolved
@tunetheweb
Copy link
Member Author

tunetheweb commented Nov 15, 2019

OK retesting on the work computer since I did the change shows this isn't going to work for companies that block access to sheets 😞

Earlier versions of this change I got working at work checked the call failed, but when checking the same code outside of work I had to tweak it to get Sheets to work when you did have access, and that then broke it for the block use case. As far as I can see there is simply no way for the browser to see whether a URL on a third-party domain is successful or not - probably for good security reasons.

I still think this is a good change to sort out iPads, and to save data for those on mobile or with data-saver enabled, albeit not as comprehensive as I thought. Plus it fixes some of the other SEO/Accessibility/Alt-tag things.

I've updated the initial description. Can you look over it again?

Copy link
Member

@rviscomi rviscomi left a comment

Choose a reason for hiding this comment

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

Sorry one more nit, then I think this is good to go.

src/static/js/chapter.js Outdated Show resolved Hide resolved
@tunetheweb
Copy link
Member Author

@rviscomi are you happy to progress this now and publish those two pages to production so can do test for real on production site? Or is there anything holding this up (other than your own time not being unlimited! 😊). I asked for some a11y help at #303 (comment) as discussed but not sure this should hold that up as can always iterate that later.

@mikegeyser if you're back off your travels and have any time, it would be good to have your opinion on this?

src/static/js/chapter.js Outdated Show resolved Hide resolved
src/static/js/chapter.js Outdated Show resolved Hide resolved
src/static/js/chapter.js Show resolved Hide resolved
@rviscomi
Copy link
Member

Let me stew in this for at most a couple more days. These are big changes and I want to make sure I think through the implications.

@tunetheweb
Copy link
Member Author

Let me stew in this for at most a couple more days. These are big changes and I want to make sure I think through the implications.

Fair enough and don't want to rush you if we do think there is any risk. Just with the translators doing more pages, plus the occasional chapter edits we see, we're just giving more clean-up work for ourselves the longer we leave it to make a call on this. Plus, the fact it is actually broken on iPads right now (and also in some corporate environments like my own - but guess that's our own fault!). And finally I'm out later in the week at PerfNow (though will bring my laptop then so maybe can discuss there as think you're there too!).

Still, those are not good enough reasons to push something in until you have comfort in it. Let me know if there's anything more I can do to give you that comfort.

@tunetheweb
Copy link
Member Author

tunetheweb commented Nov 19, 2019

@rviscomi / @mikegeyser I made one small, extra change to further de-risk this.

There is no point in calling the new chapter.js file for all the other chapters when we are only moving Accessibility and HTTP/2 chapters to the new format as part of this, for now.

Therefore I've temporarily moved that script tag out of base_chapter.html and into the markdown for both those chapters (and reverted the discussion count URL back to being inline temporarily).

This means if we merge this, then only those two chapters will be affected. We can then check it is all working in production correctly, and monitor Google Analytics to see the events being fired and then make a decision whether to do this for the rest of the chapters as part of #511 (including moving the script tag call back to base_chapter.html, along with the discussion count).

WDYT?

@rviscomi
Copy link
Member

@bazzadp I appreciate your work to minimize the risk of this issue. I'm comfortable moving this forward to a 1 or 2 chapter experiment. I'll merge and deploy the changes now.

@rviscomi rviscomi merged commit e93a550 into master Nov 19, 2019
@rviscomi rviscomi deleted the ios_fix branch November 19, 2019 19:11
@tunetheweb
Copy link
Member Author

tunetheweb commented Nov 19, 2019

Thanks mate! Good timing as was just about to leave the office but will hang round to test the fix for our corporate proxy blocking Google Sheets first! 😊

@rviscomi
Copy link
Member

The change is live! 🚀

@tunetheweb
Copy link
Member Author

And I can see visuals at work finally!! 😊

@tunetheweb
Copy link
Member Author

tunetheweb commented Nov 19, 2019

It does try to load some of the charts, until the pixel check kicks in and sees Sheets is blocked.

This is not unexpected - my pixel code deliberately kicks in AFTER all the iframes have been added and then reverts back to images when it see it's blocked. This is because I didn't want to delay loading the Interactive visuals, waiting to see whether that pixel test would load correctly or not, as this would delay the majority of cases when Sheets is not blocked just to the benefit of the minority where it is blocked. Corporates that block can suffer the extra download attempts as they will be blocked anyway. And lazy loaded so will only attempt first few charts anyway.

For other cases (e.g. on mobile screen width, on data-saver, or with not enough Canvas support) the Sheets will not be loaded first so still a good network saving for those users. Will test that when I get home.

@tunetheweb
Copy link
Member Author

Does exactly what's expected on iPads too. No Google Sheets loaded (except test pixel) and PNGs are shown. Confirmed other chapters still broken as expected.

Seeing some good hits on GA already:

  • 63/91 or 69.23% of visitors do NOT enabled interactive visualisations. So we were needless loading them for those users.
  • 52/83 or 62.65% of visitors have sheets blocked (some of them will be my tests - but not all of them).
  • 21/91 or 23.08% people have data-saver enabled (far higher than I would have thought!).

Did spot one typo where I call the event google-sheets for fails and sheets-access for successes. Will consolidate that.

I'm happy this is working as expected so will submit the full PR and you can approve whenever you're happy.

@rviscomi
Copy link
Member

62.65% of visitors have sheets blocked

This doesn't sound right.

@tunetheweb
Copy link
Member Author

I’d take it with a pinch of salt. GA can be slow to update and I ran a few tests in various browsers (maybe 20 max?) when blocked so invariably skewed the results. We’ll see over next few hours/days what the real stats are.

Wouldn’t be surprised if it was double digits though. Pesky corporates!

@tunetheweb
Copy link
Member Author

  • 21/91 or 23.08% people have data-saver enabled (far higher than I would have thought!).

Ignore this. That's how many people reported a value for data-saver (does only Chrome report this?). They all reported the value not-enabled however to say data-saving was not in effect! 😬

@tunetheweb tunetheweb added the accessibility Accessibility related label Feb 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility Accessibility related development Building the Almanac tech stack
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants