Few fixes #128

ImMorpheus · 2017-03-11T22:10:08Z

Few fixes (wip)

…ttps ?

jamierocks

This should really be commented on our OCD issue.

jamierocks · 2017-03-11T22:36:21Z

src/html/downloads.html

@@ -39,7 +39,7 @@
    <header>
        <div class="container">
            <div class="logo">
-                <img src="assets/img/icons/spongie-mark-reverse-dark.svg" />
+                <img src="assets/img/icons/spongie-mark-reverse-dark.svg" alt="Spongie reverse"/>


This really isn't required, not going to help accessibility at all.

jamierocks · 2017-03-11T22:36:27Z

src/html/include/base.html

@@ -55,7 +55,7 @@
    <div class="container">
        <div id="sp-logo-container" class="page-scroll">
            <a class="logo" href="#top">
-                <img src="assets/img/icons/spongie-mark.svg" style="height: 40px">
+                <img src="assets/img/icons/spongie-mark.svg" alt="Spongie" style="height: 40px">


This really isn't required, not going to help accessibility at all.

jamierocks · 2017-03-11T22:36:31Z

src/js/downloads/Downloads.vue

@@ -30,7 +30,7 @@
            <div class="row">
                <div class="col-lg-5 col-md-6">
                    <div class="logo">
-                        <img src="assets/img/icons/spongie-mark-reverse-dark.svg" />
+                        <img src="assets/img/icons/spongie-mark-reverse-dark.svg" alt="Spongie reverse" />


This really isn't required, not going to help accessibility at all.

jamierocks · 2017-03-11T22:36:36Z

src/js/downloads/Main.vue

@@ -30,7 +30,7 @@
            <div class="row">
                <div class="col-lg-5 col-md-6">
                    <div class="logo">
-                        <img src="assets/img/icons/spongie-mark-reverse-dark.svg" />
+                        <img src="assets/img/icons/spongie-mark-reverse-dark.svg" alt="Spongie reverse"/>


This really isn't required, not going to help accessibility at all.

ImMorpheus · 2017-03-12T10:14:12Z

The alt attribute is required for img tag. However if you really don't like the content we can use empty alt.

Edit: I forgot about the ocd list, thanks for that

jamierocks · 2017-03-12T10:25:56Z

Yes they should have an empty alt

…and sponsors page

…ed CSP on SpongeWeb

jamierocks · 2017-03-12T17:25:08Z

As of HTML5 self-closed tags (such as img) no longer need to use specific syntax (/>), and can just be left as a single tag.

ImMorpheus · 2017-03-12T17:26:31Z

Note: currently I had to add in the CSP connect-src for the download page and frame-src for kiwilrc. Everything works, if we want to make it more restrictive we can move the fonts on the server and use font-src 'self'

jamierocks · 2017-03-12T17:27:15Z

if we want to make it more restrictive we can move the fonts on the server and use font-src 'self'

That'd be overkill

jamierocks · 2017-03-12T17:34:42Z

Not to demean your pr, but these changes are the type of things that an OCD issue exists for in every SpongePowered repository (SpongeHome issue) for.

Perhaps exception can be made of this pr, in light of the number of issues resolved - however in future that is certainly where these issues should be documented.

ImMorpheus · 2017-03-12T20:22:43Z

Yeah, you've already said that (and I added the issues to the OCD list), but if you really want to fix them I can delete the pull request and no 'exception' will be made.
That being said, I fixed a few things and created this pull because I knew changes would be over 20 lines. Thank goodness small pull request get closed and people informed they should use the OCD.

jamierocks · 2017-03-12T20:30:49Z

Thank goodness small pull request get closed and people informed they should use the OCD.

In regards to that pr, I am not sure why that was merged and not directed to the OCD issue. It should have been.

jamierocks · 2017-03-12T20:32:03Z

controllers/header.go

-	"style-src 'self' 'unsafe-inline' https:; "+
-	"script-src 'self' 'unsafe-eval' https://cdnjs.cloudflare.com https://www.google-analytics.com; "+
-	"frame-ancestors 'none'")*/
+	header.Set("Content-Security-Policy", "default-src 'none'; script-src 'self' 'unsafe-inline' 'unsafe-eval' https://cdnjs.cloudflare.com/ ; style-src 'self' 'unsafe-inline' https://cdnjs.cloudflare.com; img-src 'self'; font-src https://cdnjs.cloudflare.com/ https://fonts.googleapis.com/; connect-src 'self' https://dl-api.spongepowered.org/; frame-src https://kiwiirc.com; frame-ancestors 'none'; upgrade-insecure-requests; block-all-mixed-content; ")


Can we get this split over a few lines, to promote readability?

jamierocks · 2017-03-12T20:32:56Z

src/html/downloads.html

@@ -39,7 +39,7 @@
    <header>
        <div class="container">
            <div class="logo">
-                <img src="assets/img/icons/spongie-mark-reverse-dark.svg" />
+                <img src="assets/img/icons/spongie-mark-reverse-dark.svg" alt=""/>


Change /> for > as per HTML5.

jamierocks · 2017-03-12T20:33:05Z

src/html/include/base.html

@@ -129,7 +129,7 @@
                <p>Copyright &copy; SpongePowered 2014-2017</p>
                <p id="fastly">Accelerated by<br>
                    <a href="https://www.fastly.com">
-                        <img src="assets/img/fastly.png" alt="Fastly" />
+                        <img src="assets/img/fastly.png" alt="Fastly" height="45" width="100" />


Change /> for > as per HTML5.

jamierocks · 2017-03-12T20:33:13Z

src/js/downloads/Downloads.vue

@@ -30,7 +30,7 @@
            <div class="row">
                <div class="col-lg-5 col-md-6">
                    <div class="logo">
-                        <img src="assets/img/icons/spongie-mark-reverse-dark.svg" />
+                        <img src="assets/img/icons/spongie-mark-reverse-dark.svg" alt="" />


Change /> for > as per HTML5.

jamierocks · 2017-03-12T20:33:28Z

src/js/downloads/Main.vue

@@ -30,7 +30,7 @@
            <div class="row">
                <div class="col-lg-5 col-md-6">
                    <div class="logo">
-                        <img src="assets/img/icons/spongie-mark-reverse-dark.svg" />
+                        <img src="assets/img/icons/spongie-mark-reverse-dark.svg" alt=""/>


Change /> for > as per HTML5.

jamierocks · 2017-03-12T20:34:37Z

README.md

 under the MIT license. See [LICENSE.md](LICENSE.md) for details.

 ### Branches

- `master` is hooked up to [www staging](https://www-staging.spongepowered.org/)
 - `production` is hooked up to [www prod](https://www.spongepowered.org/)


The production branch is no longer used - master is now www prod.

jamierocks · 2017-03-12T21:05:16Z

.gitignore

@@ -80,13 +80,13 @@ lib-cov
 # Coverage directory used by tools like istanbul
 coverage

-# Grunt intermediate storage (http://gruntjs.com/creating-plugins#storing-task-files)
+# Grunt intermediate storage (https://gruntjs.com/creating-plugins#storing-task-files)


May as well remove Grunt from the gitignore - we don't use it.

Is that okay ? You created that gitignore with gitignore.io, it is basically a preset which handle multiple developers using different ide/tools/etc

Yeah - Grunt is a buildtool, we use Gulp so Grunt will never become part of our dev env.

stephan-gh · 2017-03-13T12:49:05Z

.gitignore

@@ -80,13 +80,10 @@ lib-cov
 # Coverage directory used by tools like istanbul
 coverage

-# Grunt intermediate storage (http://gruntjs.com/creating-plugins#storing-task-files)


There is no reason to remove this. The gitignore is based on a template and since this isn't breaking anything in our project I see no reason to remove it.

I knew it would end up this way

stephan-gh · 2017-03-13T12:49:51Z

.gitignore

 # node-waf configuration
 .lock-wscript

-# Compiled binary addons (http://nodejs.org/api/addons.html)
+# Compiled binary addons (https://nodejs.org/api/addons.html)


Feel free to suggest this change in the upstream repository: https://github.com/github/gitignore/blob/master/Node.gitignore#L32. I don't see a reason to change it here.

stephan-gh · 2017-03-13T12:50:42Z

controllers/header.go

-	"style-src 'self' 'unsafe-inline' https:; "+
-	"script-src 'self' 'unsafe-eval' https://cdnjs.cloudflare.com https://www.google-analytics.com; "+
-	"frame-ancestors 'none'")*/
+	header.Set("Content-Security-Policy", "default-src 'none'; " +


Please move this to a separate PR, it needs careful testing and can't be accepted together with the other minor changes.

Oh ok, I didn't open a new pull because it was a rather small fix (in terms of lines)

stephan-gh · 2017-03-13T18:09:57Z

controllers/header.go

@@ -49,3 +49,54 @@ func AddHeaders(resp http.ResponseWriter) {
 		header.Add(fastly.SurrogateControlHeader, cache.SurrogateStaticContentOptions)
 	}
 }
+/*


I think this is not what you wanted to do :)

Yeah, I noticed that soon after

kashike · 2017-03-13T18:19:12Z

You should not be changing any of the sponsor images. 🙅‍♂️

ImMorpheus · 2017-03-13T18:22:35Z

I just compressed the images, can't I do that ? (Not a sensitive change, but now they are compressed)
Also, is the backgroud included in the group of images I shouldn't change ?

stephan-gh · 2017-03-13T18:54:04Z

@ImMorpheus It's probably better if we keep them as-is, because they were not made by us. Regarding the background, it seems like your optimized image has the same size as the original one?

ImMorpheus · 2017-03-13T19:05:06Z

Ok, I didn't know that. Consider it fixed
The backgroud image is 1% smaller, we are talking about nearly 500B (that's why I thought it was not a big deal)

stephan-gh · 2017-03-15T16:50:49Z

@ImMorpheus 500 bytes isn't really worth changing the image, that is almost nothing.

stephan-gh · 2017-03-15T16:51:48Z

src/html/include/base.html

@@ -129,7 +129,7 @@
                <p>Copyright &copy; SpongePowered 2014-2017</p>
                <p id="fastly">Accelerated by<br>
                    <a href="https://www.fastly.com">
-                        <img src="assets/img/fastly.png" alt="Fastly" />
+                        <img src="assets/img/fastly.png" alt="Fastly" height="45" width="100">


Why did you add an explicit width/height?

If height and width are set, the space required for the image is reserved when the page is loaded. Thus, the page loads faster.

ImMorpheus · 2017-03-15T16:56:06Z

Yeah, I compressed the images all together and there was a compression of more than 20% with some of the others. It just happened to be there and was compressed

stephan-gh · 2017-03-16T14:56:28Z

@ImMorpheus That's fine but please revert the change. We shouldn't require all clients to download the whole image again just for 500 bytes.

ImMorpheus · 2017-03-16T15:10:40Z

Done

ImMorpheus added 2 commits March 11, 2017 23:08

Hello sir, do you have a moment to talk about our lord and saviour, h…

26c90b9

…ttps ?

img tags should have an alt attribute

17d09ce

ImMorpheus changed the title ~~Switch to https~~ Few fixes Mar 11, 2017

jamierocks reviewed Mar 11, 2017

View reviewed changes

Optimized img

3d70ee8

ImMorpheus changed the title ~~Few fixes~~ [WIP] Few fixes Mar 12, 2017

ImMorpheus added 5 commits March 12, 2017 11:33

Empty alt

190699f

Specify image dimension and use the same link for staff on both home …

14c39a1

…and sponsors page

Updated link for sponge docs

9322ea1

Fixed link to the home, removed broken link to www-staging

332bea1

Cleanup, formatted code according to gofmt, goimports, goreturns, Add…

8728ab4

…ed CSP on SpongeWeb

Added CSP, fixed analytics and download page

a25ea16

jamierocks suggested changes Mar 12, 2017

View reviewed changes

Fixed /> and readme

0ab0fa0

jamierocks suggested changes Mar 12, 2017

View reviewed changes

Removed grunt from gitignore

7e4185b

stephan-gh reviewed Mar 13, 2017

View reviewed changes

Reverted gitignore and deleted csp in favour of a new pull

7b42fd4

stephan-gh reviewed Mar 13, 2017

View reviewed changes

Sorry, I'm drunk

15ddbf1

We should not be changing any of the sponsor images

e1fa410

ImMorpheus changed the title ~~[WIP] Few fixes~~ Few fixes Mar 15, 2017

stephan-gh reviewed Mar 15, 2017

View reviewed changes

Nope

73bb67c

stephan-gh approved these changes Mar 16, 2017

View reviewed changes

stephan-gh self-assigned this Mar 16, 2017

stephan-gh merged commit 580fc3f into SpongePowered:master Mar 16, 2017

Few fixes #128

Few fixes #128

Conversation

ImMorpheus commented Mar 11, 2017 • edited Loading

jamierocks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ImMorpheus commented Mar 12, 2017 • edited Loading

jamierocks commented Mar 12, 2017

jamierocks commented Mar 12, 2017

ImMorpheus commented Mar 12, 2017

jamierocks commented Mar 12, 2017

jamierocks commented Mar 12, 2017

ImMorpheus commented Mar 12, 2017

jamierocks commented Mar 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ImMorpheus Mar 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ImMorpheus Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kashike commented Mar 13, 2017

ImMorpheus commented Mar 13, 2017 • edited Loading

stephan-gh commented Mar 13, 2017

ImMorpheus commented Mar 13, 2017

stephan-gh commented Mar 15, 2017

Choose a reason for hiding this comment

ImMorpheus Mar 15, 2017 • edited Loading

Choose a reason for hiding this comment

ImMorpheus commented Mar 15, 2017

stephan-gh commented Mar 16, 2017

ImMorpheus commented Mar 16, 2017

ImMorpheus commented Mar 11, 2017 •

edited

Loading

ImMorpheus commented Mar 12, 2017 •

edited

Loading

ImMorpheus Mar 12, 2017 •

edited

Loading

ImMorpheus Mar 13, 2017 •

edited

Loading

ImMorpheus commented Mar 13, 2017 •

edited

Loading

ImMorpheus Mar 15, 2017 •

edited

Loading