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

Update dependencies #1070

Merged
merged 7 commits into from
Nov 28, 2018
Merged

Update dependencies #1070

merged 7 commits into from
Nov 28, 2018

Conversation

NickColley
Copy link
Contributor

@NickColley NickColley commented Nov 20, 2018

After updating these dependencies and running npm run build:package the autoprefixing seems to be different.

I'll need to do some cross browser testing to see the impact.

Update: I've tested cross browser, and can't see a difference...

diff --git a/package/components/footer/_footer.scss b/package/components/footer/_footer.scss
index fa6b0a53..ec004225 100644
--- a/package/components/footer/_footer.scss
+++ b/package/components/footer/_footer.scss
@@ -68,16 +68,20 @@
 
   .govuk-footer__meta {
     display: -webkit-box;
+    display: -webkit-flex;
     display: -ms-flexbox;
     display: flex; // Support: Flexbox
     margin-right: -$govuk-gutter-half;
     margin-left: -$govuk-gutter-half;
-    -ms-flex-wrap: wrap;
-        flex-wrap: wrap; // Support: Flexbox
+    -webkit-flex-wrap: wrap;
+        -ms-flex-wrap: wrap;
+            flex-wrap: wrap; // Support: Flexbox
     -webkit-box-align: end;
+    -webkit-align-items: flex-end;
         -ms-flex-align: end;
             align-items: flex-end; // Support: Flexbox
     -webkit-box-pack: center;
+    -webkit-justify-content: center;
         -ms-flex-pack: center;
             justify-content: center; // Support: Flexbox
   }
@@ -90,11 +94,13 @@
 
   .govuk-footer__meta-item--grow {
     -webkit-box-flex: 1;
+    -webkit-flex: 1;
         -ms-flex: 1;
             flex: 1; // Support: Flexbox
     @include mq ($until: tablet) {
-      -ms-flex-preferred-size: 320px;
-          flex-basis: 320px; // Support: Flexbox
+      -webkit-flex-basis: 320px;
+          -ms-flex-preferred-size: 320px;
+              flex-basis: 320px; // Support: Flexbox
     }
   }
 
@@ -154,12 +160,14 @@
 
   .govuk-footer__navigation {
     display: -webkit-box;
+    display: -webkit-flex;
     display: -ms-flexbox;
     display: flex; // Support: Flexbox
     margin-right: -$govuk-gutter-half;
     margin-left: -$govuk-gutter-half;
-    -ms-flex-wrap: wrap;
-        flex-wrap: wrap; // Support: Flexbox
+    -webkit-flex-wrap: wrap;
+        -ms-flex-wrap: wrap;
+            flex-wrap: wrap; // Support: Flexbox
   }
 
   .govuk-footer__section {
@@ -170,15 +178,18 @@
     vertical-align: top;
     // Ensure columns take up equal width (typically one-half:one-half)
     -webkit-box-flex: 1;
+    -webkit-flex-grow: 1;
         -ms-flex-positive: 1;
             flex-grow: 1; // Support: Flexbox
-    -ms-flex-negative: 1;
-        flex-shrink: 1; // Support: Flexbox
+    -webkit-flex-shrink: 1;
+        -ms-flex-negative: 1;
+            flex-shrink: 1; // Support: Flexbox
     @include mq ($until: desktop) {
       // Make sure columns do not drop below 200px in width
       // Will typically result in wrapping, and end up in a single column on smaller screens.
-      -ms-flex-preferred-size: 200px;
-          flex-basis: 200px; // Support: Flexbox
+      -webkit-flex-basis: 200px;
+          -ms-flex-preferred-size: 200px;
+              flex-basis: 200px; // Support: Flexbox
     }
   }
 
@@ -186,6 +197,7 @@
   @include mq ($from: desktop) {
     .govuk-footer__section:first-child {
       -webkit-box-flex: 2;
+      -webkit-flex-grow: 2;
           -ms-flex-positive: 2;
               flex-grow: 2; // Support: Flexbox
     }
@@ -196,20 +208,17 @@
     padding: 0;
     list-style: none;
     -webkit-column-gap: $govuk-gutter;
-       -moz-column-gap: $govuk-gutter;
             column-gap: $govuk-gutter; // Support: Columns
   }
 
   @include mq ($from: desktop) {
     .govuk-footer__list--columns-2 {
       -webkit-column-count: 2;
-         -moz-column-count: 2;
               column-count: 2; // Support: Columns
     }
 
     .govuk-footer__list--columns-3 {
       -webkit-column-count: 3;
-         -moz-column-count: 3;
               column-count: 3; // Support: Columns
     }
   }

https://trello.com/c/Sg2Uiq9z/1583-update-frontend-dependencies

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1070 November 20, 2018 14:57 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1070 November 20, 2018 15:50 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1070 November 20, 2018 17:24 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1070 November 20, 2018 17:29 Inactive
@NickColley
Copy link
Contributor Author

The new version of axe fails because we're using (what they might consider?) to be redundant roles, for example <footer role=contentinfo>, I'll roll it back for now...

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1070 November 21, 2018 10:59 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1070 November 21, 2018 11:07 Inactive
@kr8n3r
Copy link

kr8n3r commented Nov 21, 2018

some of the prefixes, like -webkit- relate to older Safari support as discussed here #1002
we've removed them there so we should not re-introduce them I think

i'm not sure about -moz ones but good to check and just only what we support

Copy link
Member

@hannalaakso hannalaakso left a comment

Choose a reason for hiding this comment

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

It looks like -webkit-flex and -webkit-align-items haven't been required since Safari 6 so it's odd that those are being added back in. Maybe something has changed with the new version of autoprefixer. Interestingly, -webkit-justify-content is required until Safari 10 so it seems sensible to have that added. I tested the footer in Safari 5 and 6 before and after changes from this PR and also can't see a difference.

Removing the -moz- ones seems legitimate as -moz-column-gap and -moz-column-count haven't been required since Firefox 51. I think 51 has probably just dropped off the list of browsers we prefix for.

The rest of the PR looks great, thanks @NickColley (as long as we're okay to have those Safari 6 prefixes added to the next package release - I don't think it should be a blocker).

Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

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

Blocking as this bumps event-stream to 3.3.6 and introduces a dependency on flatmap-stream, which is believed to be a compromised package.

@kr8n3r
Copy link

kr8n3r commented Nov 27, 2018

nodemon 1.18.7 is out that removes the affected dependencies https://github.com/remy/nodemon/releases/tag/v1.18.7

@kr8n3r kr8n3r force-pushed the update-dependencies branch from 9826be9 to a66d9f6 Compare November 27, 2018 11:16
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1070 November 27, 2018 11:19 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1070 November 27, 2018 11:46 Inactive
This is required for IE8 support
Getting some false positive failures with version 3 of axe-core that'll need to be investigated on it's own.
Since the package-lock structure has changed this test is failing
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1070 November 28, 2018 10:54 Inactive
@36degrees
Copy link
Contributor

The tests appear to be failing because package-lock.json needs updating.

Other than that, I'm happy with this.

@NickColley NickColley deleted the update-dependencies branch November 28, 2018 14:45
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.

5 participants