/www.ubuntu.com

Tidy up the local js #846

Merged
merged 4 commits into from Sep 12, 2016

Conversation

Reviewers
No reviews
Assignees

@nottrobin nottrobin

Labels
Review: Code +1 Review: QA +1
Projects
None yet
Milestone
No milestone
2 participants
@anthonydillon @nottrobin
@anthonydillon
Contributor

anthonydillon commented Sep 10, 2016 •
Edited 1 time

Done

  • Removed jQuery from every page and added it to the form pages
  • Removed import protocols
  • Replace http iframes to https

QA

Sorry this is again a long QA. I think I have now removed all YUI completely once this and the scratch branch lands. Apart from the sliders. They will require a discussion and a follow up branch.

  • Go to /about/contact-us/form and check there are no js errors. Submit an empty form and it should validate
  • Go to /cloud/contact-us and check there are no js errors. Submit an empty form and it should validate
  • Go to /cloud and check there are no js errors. Check the D3 chart renders in the "Use Ubuntu on certified public clouds" section
  • Go to /cloud/openstack and check there are no js errors.
  • Go to /cloud/openstack/managed-cloud and check there are no js errors.
  • Go to /cloud/openstack/managed-cloud/contact-us and check there are no js errors.
  • Go to /cloud/openstack/managed-cloud/thank-you you will see a jQuery error but that is resolved in the Tidy up scratch branch
  • Go to /cloud/partners/ You will find some YUI in the footer but that has been replaced in the Tidy up scratch branch
  • Go to /cloud/storage/contact-us and you should find no js errors
  • Go to /cloud/thank-you and you should see a missing jQuery dep. This function is coming from scratch and has been rewritten in ES5 in the Tidy up scratch branch
  • Go to /cloud/training/classroom-contact-us and check there are no js errors
  • Go to /cloud/training/contact-us and there should be no js errors
  • Go to /cloud/training/onsite-contact-us and check there are no errors
  • Go to /cloud/training/server-admin-onsite-contact-us and check there are no js errors
  • Go to /cloud/training/thank-you and you will see a jQuery in scratch that is resolved with the Tidy up scratch branch
  • Go to /desktop/contact-us and you should see no errors
  • Go to /desktop/features and scroll to the 'Photos' section and it should have a background image with a caption in the bottom right. There will be a jQuery error from scratch but that is fixed in the scratch branch
  • Go to /desktop/partners and scroll to 'A SELECTION OF OUR DESKTOP PARTNERS' and check there is a list of logos
  • Go to /desktop/tenders-contact-us and check there are no js errors and the validation works
  • Go to /download/desktop/thank-you?version=16.04.1&architecture=amd64 and check the download starts
  • Go to /download/server/thank-you-linuxone and check there are no errors
  • Go to /download/server/thank-you?version=16.04.1&architecture=amd64 and check the download kicks off. There will be a jQuery error.
  • Go to /internet-of-things/contact-us and chec kthere is no errors.
  • Go to /internet-of-things/digital-signage/contact-us and check there are no errors.
  • Go to /internet-of-things/digital-signage/thank-you and check there is just the jquery scratch error
  • Go to /legal/contributors/submit and check you get the same errors as live
  • Go to /legal/terms-and-policies/contact-us and check you get the same errors as live
  • Go to /management/contact-us and check there are no errors and the validation works
  • Go to /server/contact-us and check there is no errors
  • Go to /tablet/contact-us and check for errors

@anthonydillon anthonydillon added the Status: Review label Sep 10, 2016

@nottrobin nottrobin added this to the HTTPS milestone Sep 10, 2016

templates/internet-of-things/thank-you.html
@@ -33,11 +33,11 @@
var google_conversion_value = 0;
/* ]]> */
</script>
-<script type="text/javascript" src="http://www.googleadservices.com/pagead/conversion.js">
+<script type="text/javascript" src="//www.googleadservices.com/pagead/conversion.js">
@nottrobin

nottrobin Sep 10, 2016

Owner

Could we explicitly use HTTPS urls? Protocolless URLs are now (arguably) an antipattern. https://twitter.com/paul_irish/status/588502455530311680. Credit to @barrymcgee.

@anthonydillon

anthonydillon Sep 10, 2016

Contributor

Okie doke

templates/internet-of-things/thank-you.html
</script>
<noscript>
<div style="display:inline;">
-<img height="1" width="1" style="border-style:none;" alt="" src="http://www.googleadservices.com/pagead/conversion/1012391776/?value=0&amp;label=-mBBCIC5vwMQ4L7f4gM&amp;guid=ON&amp;script=0"/>
+<img height="1" width="1" style="border-style:none;" alt="" src="//www.googleadservices.com/pagead/conversion/1012391776/?value=0&amp;label=-mBBCIC5vwMQ4L7f4gM&amp;guid=ON&amp;script=0"/>
@nottrobin

nottrobin Sep 10, 2016

Owner

Ditto

templates/management/thank-you.html
@@ -22,11 +22,11 @@
var google_conversion_value = 0;
/* ]]> */
</script>
-<script type="text/javascript" src="http://www.googleadservices.com/pagead/conversion.js">
+<script type="text/javascript" src="//www.googleadservices.com/pagead/conversion.js">
@nottrobin

nottrobin Sep 10, 2016

Owner

HTTPS please

templates/management/thank-you.html
</script>
<noscript>
<div style="display:inline;">
-<img height="1" width="1" style="border-style:none;" alt="" src="http://www.googleadservices.com/pagead/conversion/1012391776/?value=0&amp;label=utP4CMDOwQMQ4L7f4gM&amp;guid=ON&amp;script=0"/>
+<img height="1" width="1" style="border-style:none;" alt="" src="//www.googleadservices.com/pagead/conversion/1012391776/?value=0&amp;label=utP4CMDOwQMQ4L7f4gM&amp;guid=ON&amp;script=0"/>
@nottrobin

nottrobin Sep 10, 2016

Owner

HTTPS please

templates/tablet/thank-you.html
@@ -36,11 +36,11 @@
var google_conversion_value = 0;
/* ]]> */
</script>
-<script type="text/javascript" src="http://www.googleadservices.com/pagead/conversion.js">
+<script type="text/javascript" src="//www.googleadservices.com/pagead/conversion.js">
@nottrobin

nottrobin Sep 10, 2016

Owner

HTTPS please

templates/tablet/thank-you.html
</script>
<noscript>
<div style="display:inline;">
-<img height="1" width="1" style="border-style:none;" alt="" src="http://www.googleadservices.com/pagead/conversion/1012391776/?value=0&amp;label=-mBBCIC5vwMQ4L7f4gM&amp;guid=ON&amp;script=0"/>
+<img height="1" width="1" style="border-style:none;" alt="" src="//www.googleadservices.com/pagead/conversion/1012391776/?value=0&amp;label=-mBBCIC5vwMQ4L7f4gM&amp;guid=ON&amp;script=0"/>
@nottrobin

nottrobin Sep 10, 2016

Owner

HTTPS please

templates/desktop/tenders/contact-us.html
@@ -1,146 +0,0 @@
-{% extends "desktop/base_desktop.html" %}
@nottrobin

nottrobin Sep 10, 2016

Owner

Isn't this page needed? It's linked to from http://127.0.0.1:8001/desktop/partners (although that page should be using a relative rather than absolute URL).

@nottrobin

nottrobin Sep 10, 2016 •
Edited 1 time

Owner

Oh no sorry I see, it redirects to /desktop/tenders-contact-us.html. 👍

templates/about/contact-us/form/thank-you.html
@@ -43,11 +43,11 @@
var google_conversion_value = 0;
/* ]]> */
</script>
-<script type="text/javascript" src="http://www.googleadservices.com/pagead/conversion.js">
+<script type="text/javascript" src="//www.googleadservices.com/pagead/conversion.js">
@nottrobin

nottrobin Sep 10, 2016

Owner

HTTPS please

templates/about/contact-us/form/thank-you.html
</script>
<noscript>
<div style="display:inline;">
-<img height="1" width="1" style="border-style:none;" alt="" src="http://www.googleadservices.com/pagead/conversion/1012391776/?value=0&amp;label=utP4CMDOwQMQ4L7f4gM&amp;guid=ON&amp;script=0"/>
+<img height="1" width="1" style="border-style:none;" alt="" src="//www.googleadservices.com/pagead/conversion/1012391776/?value=0&amp;label=utP4CMDOwQMQ4L7f4gM&amp;guid=ON&amp;script=0"/>
@nottrobin

nottrobin Sep 10, 2016

Owner

HTTPS please

templates/cloud/thank-you.html
@@ -9,7 +9,7 @@
{% block content %}
{% include "shared/_thank_you.html" with thanks_context="enquiring about Cloud" %}
-
+
{% endblock content %}
{% block footer_extra %}
<!-- Google Code for Services Canonical &mdash; contact us Conversion Page -->
@nottrobin

nottrobin Sep 10, 2016

Owner

I'm pretty sure this block of code for google ad services is exactly duplicated many times throughout the codebase. It could probably benefit from being an include.

@anthonydillon

anthonydillon Sep 10, 2016

Contributor

Yea spotted that. Alot of moving code in this branch. Will do a polish branch after.

templates/cloud/thank-you.html
@@ -23,11 +23,11 @@
var google_conversion_value = 0;
/* ]]> */
</script>
-<script type="text/javascript" src="http://www.googleadservices.com/pagead/conversion.js">
+<script type="text/javascript" src="//www.googleadservices.com/pagead/conversion.js">
@nottrobin

nottrobin Sep 10, 2016

Owner

HTTPS please

templates/cloud/thank-you.html
</script>
<noscript>
<div style="display:inline;">
-<img height="1" width="1" style="border-style:none;" alt="" src="http://www.googleadservices.com/pagead/conversion/1012391776/?value=0&amp;label=-mBBCIC5vwMQ4L7f4gM&amp;guid=ON&amp;script=0"/>
+<img height="1" width="1" style="border-style:none;" alt="" src="//www.googleadservices.com/pagead/conversion/1012391776/?value=0&amp;label=-mBBCIC5vwMQ4L7f4gM&amp;guid=ON&amp;script=0"/>
@nottrobin

nottrobin Sep 10, 2016

Owner

HTTPS please

templates/cloud/training/thank-you.html
@@ -25,11 +25,11 @@
var google_conversion_value = 0;
/* ]]> */
</script>
-<script type="text/javascript" src="http://www.googleadservices.com/pagead/conversion.js">
+<script type="text/javascript" src="//www.googleadservices.com/pagead/conversion.js">
@nottrobin

nottrobin Sep 10, 2016

Owner

HTTPS please

templates/cloud/training/thank-you.html
</script>
<noscript>
<div style="display:inline;">
-<img height="1" width="1" style="border-style:none;" alt="" src="http://www.googleadservices.com/pagead/conversion/1012391776/?value=0&amp;label=-mBBCIC5vwMQ4L7f4gM&amp;guid=ON&amp;script=0"/>
+<img height="1" width="1" style="border-style:none;" alt="" src="//www.googleadservices.com/pagead/conversion/1012391776/?value=0&amp;label=-mBBCIC5vwMQ4L7f4gM&amp;guid=ON&amp;script=0"/>
@nottrobin

nottrobin Sep 10, 2016

Owner

HTTPS please

templates/desktop/partners.html
@@ -121,8 +121,10 @@ <h2 class="twelve-col">What our partners are saying</h2>
{% endblock content %}
{% block footer_extra %}
<script>
- partnersApiParams = '?programme__name=Desktop&featured=true';
- partnersElementId = '#hardware-partners';
+ partnerLogoClouds = [{
@nottrobin

nottrobin Sep 10, 2016

Owner

As previously discussed, could we change this to being a function call?

templates/desktop/shared/_share-this.html
@@ -1,5 +1,5 @@
<div class="share-this">
- <iframe src="http://www.facebook.com/plugins/like.php?href=http%3A%2F%2Fwww.ubuntu.com%2Fubuntu&amp;send=false&amp;layout=button_count&amp;show_faces=false&amp;action=like&amp;colorscheme=light&amp;font&amp;height=20" style="border:none; overflow:hidden; height:21px; padding-right:23px;"></iframe>
+ <iframe src="//www.facebook.com/plugins/like.php?href=http%3A%2F%2Fwww.ubuntu.com%2Fubuntu&amp;send=false&amp;layout=button_count&amp;show_faces=false&amp;action=like&amp;colorscheme=light&amp;font&amp;height=20" style="border:none; overflow:hidden; height:21px; padding-right:23px;"></iframe>
@nottrobin

nottrobin Sep 10, 2016

Owner

HTTPS please

templates/download/server/linuxone-signup-thank-you.html
@@ -37,11 +37,11 @@
var google_conversion_value = 0;
/* ]]> */
</script>
-<script type="text/javascript" src="http://www.googleadservices.com/pagead/conversion.js">
+<script type="text/javascript" src="//www.googleadservices.com/pagead/conversion.js">
@nottrobin

nottrobin Sep 10, 2016

Owner

HTTPS please

templates/download/server/linuxone-signup-thank-you.html
</script>
<noscript>
<div style="display:inline;">
- <img height="1" width="1" style="border-style:none;" alt="" src="http://www.googleadservices.com/pagead/conversion/1012391776/?value=0&amp;label=utP4CMDOwQMQ4L7f4gM&amp;guid=ON&amp;script=0"/>
+ <img height="1" width="1" style="border-style:none;" alt="" src="//www.googleadservices.com/pagead/conversion/1012391776/?value=0&amp;label=utP4CMDOwQMQ4L7f4gM&amp;guid=ON&amp;script=0"/>
@nottrobin

nottrobin Sep 10, 2016

Owner

HTTPS please

templates/internet-of-things/digital-signage/thank-you.html
@@ -33,11 +33,11 @@
var google_conversion_value = 0;
/* ]]> */
</script>
-<script type="text/javascript" src="http://www.googleadservices.com/pagead/conversion.js">
+<script type="text/javascript" src="//www.googleadservices.com/pagead/conversion.js">
@nottrobin

nottrobin Sep 10, 2016

Owner

HTTPS please

templates/internet-of-things/digital-signage/thank-you.html
</script>
<noscript>
<div style="display:inline;">
-<img height="1" width="1" style="border-style:none;" alt="" src="http://www.googleadservices.com/pagead/conversion/1012391776/?value=0&amp;label=-mBBCIC5vwMQ4L7f4gM&amp;guid=ON&amp;script=0"/>
+<img height="1" width="1" style="border-style:none;" alt="" src="//www.googleadservices.com/pagead/conversion/1012391776/?value=0&amp;label=-mBBCIC5vwMQ4L7f4gM&amp;guid=ON&amp;script=0"/>
@nottrobin

nottrobin Sep 10, 2016

Owner

HTTPS please

templates/internet-of-things/partners.html
- core.loadPartners('?programme__name=Internet%20of%20Things', '#iot-partners');
- });
- });
+ partnerLogoClouds = [{
@nottrobin

nottrobin Sep 10, 2016

Owner

I'm guessing this ain't gonna work without partners.js being loaded afterwards.

In any case, let's move this to a function call instead.

@nottrobin
Owner

nottrobin commented Sep 10, 2016 •
Edited 1 time

Wanna do this as a driveby? ubuntudesign#847. Ignore if you want.
@anthonydillon
Contributor

anthonydillon commented Sep 10, 2016

Thanks for the review @nottrobin. Rebased and explicitly using HTTPS urls throughout.

@nottrobin nottrobin self-assigned this Sep 11, 2016

@anthonydillon
Contributor

anthonydillon commented Sep 11, 2016

TL; DR. Na, forms need a rethink.

I want to bring up the number of forms across the site and the different methods they have been implemented. I think as they seem to make up such a large proportion of the site need to develop a elegant and consistent method.
@nottrobin
Owner

nottrobin commented Sep 11, 2016

Lol how could anyone find your 1 paragraph too long?

@nottrobin nottrobin referenced this pull request Sep 12, 2016

Closed

Standardise form implementations #851

@nottrobin
Owner

nottrobin commented Sep 12, 2016

@anthonydillon I filed your forms point as issue #851.

@anthonydillon anthonydillon added Review: QA needed Review: Code needed labels Sep 12, 2016

static/js/partners.js
@@ -15,7 +15,7 @@ var partners = {}
/**
* Loops through the partner clouds and loads the feeds for each.
*/
-partners.loadPartnerClouds = function() {
+partners.loadPartnerClouds = function(partnerLogoClouds) {
@nottrobin

nottrobin Sep 12, 2016 •
Edited 1 time

Owner

I would rather this was (as mentioned before):

partners.loadPartnerCloud = function (queryParams, elementId) {

But if you want to merge this for now I can tidy it up afterwards.

static/js/partners.js
-} else {
- console.warn('Load partner logos: Expected array "partnerLogoClouds" not found. Stopping.');
-}
+// if (typeof(partnerLogoClouds) === 'object') {
@nottrobin

nottrobin Sep 12, 2016

Owner

Likewise, it would be nice not to keep commented out code, but I'm happy to tidy this up myself afterwards, since I need to do some work for the managed cloud calculator anyway,

@nottrobin
Owner

nottrobin commented Sep 12, 2016

@anthonydillon thanks for doing that. But templates/server/index.html still contains a partnerLogoClouds variable, which presumably won't work now.
@nottrobin
Owner

nottrobin commented Sep 12, 2016 •
Edited 1 time

http://127.0.0.1:8001/cloud/training/onsite-contact-us doesn't seem to be using the validate library. I just see HTML5 validation in Firefox. Is this what you meant by forms being inconsistent? Or is it unintentional?

Same with http://127.0.0.1:8001/cloud/training/server-admin-onsite-contact-us.

EDIT: Ah yes, 'tis the same on live. 👍

@nottrobin nottrobin added Review: QA +1 Review: Code +1 and removed Review: Code needed Review: QA needed labels Sep 12, 2016

@nottrobin
Owner

nottrobin commented Sep 12, 2016

All LGTM and works. Great work! Merging.

@nottrobin nottrobin merged commit fd5efac into canonical-websites:master Sep 12, 2016

@nottrobin nottrobin removed the Status: Review label Sep 12, 2016

@anthonydillon anthonydillon deleted the anthonydillon:tidy-up-local-js branch Apr 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment