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

Load third-party libraries using Cdnjs #2427

Merged
merged 17 commits into from
Mar 13, 2017
Merged

Conversation

stephengroat
Copy link
Contributor

Include semantic ui from cdn.

still need a tiny bit of help, my font-size override trick works for everything except the icon in the header

@mxxcon
Copy link
Contributor

mxxcon commented Mar 6, 2017

Let's implement SRI tags. You can use https://www.srihash.org/ to speed things up.

@jamcat22 jamcat22 added the enhancement Issue/PR contains enhancements to the overall code of the site. label Mar 6, 2017
@stephengroat
Copy link
Contributor Author

stephengroat commented Mar 6, 2017 via email

@fpigerre fpigerre added this to the Remove Semantic-UI milestone Mar 6, 2017
@fpigerre
Copy link
Contributor

fpigerre commented Mar 6, 2017

Awesome stuff @stephengroat! 👍

At the moment, the Semantic-UI library sets the icon font-size to 2em through the following classes:
.ui.icon.header .circular.icon, .ui.icon.header .square.icon {
It's quite likely that somewhere, either in our own CSS or in Semantic-UI's CSS, there is a line of code that sets a base font-size lower than 18px specifically for a class which affects the logo icon. I'll have a dig around over the next day or two and see what I can find.
You can also see exactly which classes have what effects on particular elements by right-clicking on an element and selecting "Inspect". From there, specific classes provided for by the CSS appear in the Styles tab. The "Computed" tab is also really useful, as it shows the whole collection of styles applied to that specific element, regardless of how they were applied.

Would you be able to re-factor the style="font-size:18px" snippet into the css/base.scss file by any chance? This helps keep all the CSS in one place.

Thanks,
psgs 🌴

Copy link
Contributor

@fpigerre fpigerre left a comment

Choose a reason for hiding this comment

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

  • Refactor style tag into base.scss
  • Fix logo icon sizing
  • Implement SRI hashes

@stephengroat
Copy link
Contributor Author

@PSGS thanks for the help, i don't really know what i'm doing with css (learning curve is steep).

i moved the font-size to the css and it seems to fix the icon issue, could you look the size over and see if you can tell any major differences? i tried it on chrome and firefox and it looks the same (to me)

@fpigerre
Copy link
Contributor

fpigerre commented Mar 7, 2017

Awesome work @stephengroat! 👍

The only difference with the sizing of elements that I can see is a slight difference in the width of table columns. It's probably caused by several elements gradually inheriting a larger font size. Visually, the difference is negligible, however, so I think we can go ahead and merge the pull request.

@Carlgo11, @mxxcon: Should we specify one particular type of hash to use for SRI hashes (eg. sha-256), or allow different hash types to be used for each resource?

@fpigerre fpigerre changed the title Cdnjs Load third-party libraries using Cdnjs Mar 7, 2017
@mxxcon
Copy link
Contributor

mxxcon commented Mar 7, 2017

Should we specify one particular type of hash to use for SRI hashes (eg. sha-256), or allow different hash types to be used for each resource?

I think it doesn't really matter as long as they are secure hashes(not sha1/md5). We could stick to one type just for consistency sake.

@jamcat22
Copy link
Member

jamcat22 commented Mar 7, 2017

I think if we decide to stick to one type, we should use sha-384, since that's what https://www.srihash.org uses when generating hashes.

@stephengroat
Copy link
Contributor Author

stephengroat commented Mar 7, 2017 via email

@stephengroat
Copy link
Contributor Author

@PSGS i'm still seeing a problem for the backup entry pogoplug

do you see anything?

@fpigerre
Copy link
Contributor

fpigerre commented Mar 8, 2017

@stephengroat, I can't seem to see any problems with Pogoplug.
Do the screenshots below seem right to you?

screen shot 2017-03-08 at 22 17 27

screen shot 2017-03-08 at 22 17 09

@stephengroat
Copy link
Contributor Author

stephengroat commented Mar 8, 2017

@PSGS you're 100% right. i was comparing against an unmerged live copy that still had the in progress badge on it and was getting VERY confused!

@stephengroat
Copy link
Contributor Author

@2factorauth/collaborators can some more people look this over? i think this one needs more than 2 reviews

@@ -16,8 +16,10 @@

<title>{{ site.title }}</title>

<link rel="stylesheet" href="//fonts.googleapis.com/css?family=Open+Sans:400,700">
<link rel="stylesheet" href="/lib/semantic/css/semantic.min.css">
<link rel="stylesheet" href="https://fonts.googleapis.com/css?family=Open+Sans:400,700"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should NOT use SRI for Google Fonts. Apparently CSS they serve is intentionally unversioned and it changes depending on which browser requests it.
See google/fonts#473

Copy link
Member

@jamcat22 jamcat22 left a comment

Choose a reason for hiding this comment

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

Is it possible to change the hashes of our jquery source to be sha384?

@jamcat22 jamcat22 merged commit 3dc4fbe into 2factorauth:master Mar 13, 2017
@fpigerre
Copy link
Contributor

👍

@stephengroat stephengroat deleted the cdnjs branch March 14, 2017 02:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issue/PR contains enhancements to the overall code of the site.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants