Clean up #173

jerone · 2014-06-15T22:15:44Z

I'm doing some clean up to all files that need it. This mostly consists of validating the HTML and keep all files consistent.

~~It is still work in progress. I hope to finish it tomorrow.~~ Finished.

* Added image dimensions; * Added message to leave image alternate text always empty;

* Consistency;

* Removed unneeded mustache if;

* Consistency;

Martii · 2014-06-15T23:35:48Z

May we work towards HTML5 validation too? Any used , , etc. should probably not be used in our view pages. , , etc. are current.

Martii · 2014-06-16T01:03:39Z

I'm leaning towards -1 so far ... because of the & is HTML Entitied for href values. e.g. & This looks fugly and user.js is going to have to put in special @*cludes that are harder to read. http://validator.w3.org uses very vague summaries of:

(& probably should have been escaped as &.)

Strange nomenclature on that site too.. escaped isn't the same as HTML Entities.

http://userscripts.org:8080/posts?kind=all&spam=1
vs.
http://userscripts.org:8080/posts?kind=all&spam=1

result: Different results returned.

4396 page differences 😨 found between:
https://github.com/OpenUserJs/OpenUserJS.org/issues?labels=sooner&state=open
and
https://github.com/OpenUserJs/OpenUserJS.org/issues?labels=sooner&page=1&state=open

result: Same visual results but definite source page differences... this implies that GH has special handlers for this unusual HTML Entity processing.

I also seem to recall there is an alternate to & but I can't recall what it is but I think it was ; when I read it... EDIT: Found it at http://www.w3.org/TR/1999/REC-html401-19991224/appendix/notes.html#h-B.2.2

Zren · 2014-06-16T01:30:39Z

encodeURIComponent("all&spam=1")
"all%26spam%3D1"

Zren · 2014-06-16T01:36:55Z

views/pages/modPage.html

@@ -15,7 +15,7 @@
          <a href="/?flagged=true" class="list-group-item">
            <i class="fa fa-fw fa-file-code-o"></i> <i class="fa fa-fw fa-flag"></i> Flagged Scripts
          </a>
-          <a href="/?library=true&flagged=true" class="list-group-item">
+          <a href="/?library=true&amp;flagged=true" class="list-group-item">


Flagged and library are separate query parameters. You don't escape them the & as it then becomes amp;flagged = true. Everywhere you've done this in this PR (inside a uri) is unnecessary.

Flagged and library are separate query parameters.

Does this part mean you are pointing out a bug with both QSPs being in here or just the & issue?

jerone · 2014-06-16T17:57:17Z

Seeing a few comments about encoding the ampersand entity (& -> &). I wouldn't go as far that it's illegal, but in any case it's bad practice! A lot of talk about it over the Internet; e.g. http://stackoverflow.com/questions/3493405/do-i-really-need-to-encode-as-amp

To be clear, encoding entities doesn't have any effect to the link. You'll only see the & when you are viewing the page source in your browser. As soon as the page is rendered, they are changed in the normal ampersand! Have a try with your browser developer tool. Links like http://userscripts.org:8080/posts?kind=all&spam=1 will thus never happen.

The upside is in making better code & browsers will read it as it should have been (instead of fixing bad coding).

Please comment. If people have a valid counter argument or strong opinion to not use it, I'm happy to undo all & to keep the other changes. Any decision we make about this should go into the style-guide.

jerone · 2014-06-16T18:35:21Z

May we work towards HTML5 validation too? Any used , , etc. should probably not be used in our view pages. , , etc. are current.

Aldo  and  are valid HTML5 elements, their semantic counterparts ( and ) are more appropriate. But still they shouldn't be used as both are part of the site style, and should be changed to classes and styled in the stylesheet.

... etc. ...

What other elements are you referring to?

Martii · 2014-06-16T19:17:25Z

What other elements are you referring to?

Anything that I may have missed.

This is the definitive source for HTML5 and no others... while there are a lot of opinions on the internet this is where it is all gathered and defined succinctly.

http://www.w3.org/International/questions/qa-b-and-i-tags

strong opinion to not use it

I vote we don't use HTML Entities in URI/URLs. As I stated already above authors will need to craft specifc 'cludes and handlers just to accommodate it. Your citations are random web pages for it too which aren't always correct as we've all seen... unfortunately I can't find much on the RFC back in the day when the proposal was drafted but I am still looking for it assuming it exists. If I am outvoted then with this change you must provide the URI/URL transformation handling routine for GM as well as OUJS.... Do you really want to do that?

jerone · 2014-06-16T22:01:06Z

As I stated already above authors will need to craft specifc 'cludes and handlers just to accommodate it.

http://localhost:1337/?orderBy=updated&orderDir=desc
is exactly the same as
http://localhost:1337/?orderBy=updated&orderDir=desc

http://localhost:1337/?orderBy=updated&orderDir=desc will only be seen when viewing the source.
http://localhost:1337/?orderBy=updated&orderDir=desc is what will be rendered by the browser and seen by JavaScript and thus seen by GreaseMonkey (or other addons).

Please try it in your development branch.

Zren · 2014-06-16T22:41:41Z

doesn't have any effect to the link

My bad, tried it on JSFiddle. TIL.

Wondering if we need to user {{url}} instead of {{{url}}} in our mustache templates.

<script id="template" type="x-tmpl-mustache">
  <a href="{{url}}">Escaped Link</a>
  <a href="{{{url}}}">Unescaped Link</a>
</script>

var template = $('#template').html();
Mustache.parse(template);   // optional, speeds up future uses
var options = {
  url: "https://openuserjs.org/?library=true&flagged=true",
};
var rendered = Mustache.render(template, options);
console.log(rendered);

  <a href="https:&#x2F;&#x2F;openuserjs.org&#x2F;?library=true&amp;flagged=true">Escaped Link</a>
  <a href="https://openuserjs.org/?library=true&flagged=true">Unescaped Link</a>

This might cause an annoyance with rendering the pagination widget, as we need to user the {{{ }}} to not escape the <>.

Martii · 2014-06-16T23:33:25Z

I'm still toward -1 for HTML Entities in href attributes regardless. The spec is fuzzy and not cited yet. Until it's found I personally don't care how it's been discussed on third party sites... and yes I could use some help finding the spec... I spent several hours searching for it with no avail. The IETF RFC 3986§3.4 is not as complete as I would have liked it to be.

if we need to user {{url}} instead of {{{url}}} in our mustache templates.

I couldn't get these to do anything different in some other issues during testing and could be related to #62 and Martii@aaab607 ... but perhaps my input data didn't need the change. IDK.

sizzlemctwizzle · 2014-06-17T00:25:11Z

I see no real advantage or reason to HTML entity escape query strings.

jerone · 2014-06-17T14:49:13Z

Is it useful to continue with this PR?

Martii · 2014-06-17T18:32:10Z

Well if you backout the & bit you do seem to have some other excellent cleanups from the other day. Did you fix those elsewhere? Your contributions are definitely a plus in my book... sometimes we all need some confirmation with discussions. :)

sizzlemctwizzle · 2014-06-17T19:39:13Z

Is it useful to continue with this PR?

I agree with @Martii. I want to merge this pull request, but I don't want the entities escaping on urls.

* Consistency;

jerone · 2014-06-18T11:09:20Z

I've done all HTML pages I'm happy in cleaning up. Some pages require another review to get them valid.

Ready for review or merge...

Martii · 2014-06-18T11:15:08Z

Does sizzle need to merge #179 and/or #182 first before doing this one so it doesn't make any of them stale with a merge conflict resolution needed?

WOW! You found a few boogs (bugs).

jerone · 2014-06-18T11:18:27Z

Does sizzle need to merge #179 and/or #182 first before doing this one so it doesn't make any of them stale?

I can't tell you. This is a big merge, so maybe the other PR's are easier to fix conflicts :)

Martii · 2014-06-18T11:28:08Z

so maybe the other PR's are easier to fix conflicts

Probably (e.g. recommend doing this one first and then those since they are less lines @sizzlemctwizzle) ... still zipping through your changes... just stopped in here to check for response.

jerone · 2014-06-18T11:48:05Z

still zipping through your changes

Yeah its a lot 😃

Martii · 2014-06-18T11:49:35Z

Alright... up to this point ya got a +1 from me :) Nice catches here... plus you saved me from doing this! GRIN The end of tabs I hope... at least in the html files.

* Removed one unused variable;

jerone · 2014-06-18T17:57:11Z

I just added 2 commits for app.js and all controller to clean them (mostly spaces).

jerone · 2014-06-18T18:14:51Z

Done libs and models too.

Now I'm completely done. Ready for review or merge...

jerone · 2014-06-18T18:19:55Z

For reviewing... adding ?w=1 at the end of the PR files will show you more the real changes without most whitespace changes: https://github.com/OpenUserJs/OpenUserJS.org/pull/173/files?w=1

sizzlemctwizzle · 2014-06-18T20:44:40Z

controllers/admin.js

@@ -42,28 +42,33 @@ function getOAuthStrategies(stored) {
 }

 // Allow admins to set user roles and delete users
-exports.userAdmin = function (req, res, next) {
+exports.userAdmin = function(req, res, next) {


Why are getting rid of that space? Our style guide states that we put that space in there.

Oke missed that one. What do you guys want me to do?

Please put the spaces back in for inline anonymous functions for now. I know this might be tedious and may need another check after merge.

Martii · 2014-06-18T21:02:46Z

Arggh... it says "We can't automatically merge this pull request." now. :\

sizzlemctwizzle · 2014-06-18T21:03:55Z

it says "We can't automatically merge this pull request." now.

I know. I've already had to do manual conflict resolution for the last pull request I merged.

jerone · 2014-06-18T21:13:49Z

Did a merge of the upstream master branch. All fine now.

…into hci Conflicts: app.js controllers/discussion.js controllers/group.js controllers/issue.js controllers/script.js controllers/scriptStorage.js controllers/user.js libs/markdown.js libs/modelParser.js views/includes/discussionList.html

jerone · 2014-06-22T10:27:34Z

Just did another merge of the upstream master branch to solve a lot of conflicts. Probably won't be doing that a third time.

After this PR merge, I'll make another one to conform to the style guide.

Clean up

jerone added 8 commits June 15, 2014 22:27

* Valid HTML;

5f9a5fc

* Added image dimensions; * Added message to leave image alternate text always empty;

* Valid HTML;

7114acb

* Consistency;

* Valid HTML;

ae9e682

* Removed unneeded mustache if;

* Used HTML entity for copyright symbol;

8ee7f9f

* Valid HTML;

e04b374

* Consistency;

* Valid HTML;

4d41e88

* Consistency;

* Valid HTML;

2a87175

* Consistency;

Merge branch 'hci' of https://github.com/jerone/OpenUserJS.org into hci

a0c33e7

Zren reviewed Jun 16, 2014
View reviewed changes

Martii mentioned this pull request Jun 16, 2014

Fix support for @homepage and upmix to @homepageURL for symmetry #174

Merged

jerone added 2 commits June 18, 2014 12:02

Revert ampersand entities in URLs;

f334c7a

* Valid HTML;

2596114

* Consistency;

jerone added 2 commits June 18, 2014 19:17

* Consistency;

4720f4c

* Removed one unused variable;

Consistency;

3a780f2

Consistency;

e2e8763

Martii mentioned this pull request Jun 18, 2014

dev: Fix 404 message for missing favicon.ico #184

Closed

Consistency;

e1eafdb

sizzlemctwizzle reviewed Jun 18, 2014
View reviewed changes

More Consistency;

9f8a25d

Merge branch 'master' into hci;

acd6eb5

sizzlemctwizzle added a commit that referenced this pull request Jun 22, 2014

Merge pull request #173 from jerone/hci

d2b9719

Clean up

sizzlemctwizzle merged commit d2b9719 into OpenUserJS:master Jun 22, 2014

Martii mentioned this pull request Jun 24, 2014

UTF-8, MIME Type Declaration, BOMs, and other encoding standards #198

Closed

jerone deleted the hci branch August 22, 2014 07:19

github-actions bot locked as resolved and limited conversation to collaborators May 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up #173

Clean up #173

jerone commented Jun 15, 2014

Martii commented Jun 15, 2014

Martii commented Jun 16, 2014

Zren commented Jun 16, 2014

Zren Jun 16, 2014

Martii Jun 16, 2014

jerone commented Jun 16, 2014

jerone commented Jun 16, 2014

Martii commented Jun 16, 2014

jerone commented Jun 16, 2014

Zren commented Jun 16, 2014

Martii commented Jun 16, 2014

sizzlemctwizzle commented Jun 17, 2014

jerone commented Jun 17, 2014

Martii commented Jun 17, 2014

sizzlemctwizzle commented Jun 17, 2014

jerone commented Jun 18, 2014

Martii commented Jun 18, 2014

jerone commented Jun 18, 2014

Martii commented Jun 18, 2014

jerone commented Jun 18, 2014

Martii commented Jun 18, 2014

jerone commented Jun 18, 2014

jerone commented Jun 18, 2014

jerone commented Jun 18, 2014

sizzlemctwizzle Jun 18, 2014

Martii Jun 18, 2014

jerone Jun 18, 2014

Martii Jun 18, 2014

Martii commented Jun 18, 2014

sizzlemctwizzle commented Jun 18, 2014

jerone commented Jun 18, 2014

jerone commented Jun 22, 2014

Clean up #173

Clean up #173

Conversation

jerone commented Jun 15, 2014

Martii commented Jun 15, 2014

Martii commented Jun 16, 2014

Zren commented Jun 16, 2014

Zren Jun 16, 2014

Choose a reason for hiding this comment

Martii Jun 16, 2014

Choose a reason for hiding this comment

jerone commented Jun 16, 2014

jerone commented Jun 16, 2014

Martii commented Jun 16, 2014

jerone commented Jun 16, 2014

Zren commented Jun 16, 2014

Martii commented Jun 16, 2014

sizzlemctwizzle commented Jun 17, 2014

jerone commented Jun 17, 2014

Martii commented Jun 17, 2014

sizzlemctwizzle commented Jun 17, 2014

jerone commented Jun 18, 2014

Martii commented Jun 18, 2014

jerone commented Jun 18, 2014

Martii commented Jun 18, 2014

jerone commented Jun 18, 2014

Martii commented Jun 18, 2014

jerone commented Jun 18, 2014

jerone commented Jun 18, 2014

jerone commented Jun 18, 2014

sizzlemctwizzle Jun 18, 2014

Choose a reason for hiding this comment

Martii Jun 18, 2014

Choose a reason for hiding this comment

jerone Jun 18, 2014

Choose a reason for hiding this comment

Martii Jun 18, 2014

Choose a reason for hiding this comment

Martii commented Jun 18, 2014

sizzlemctwizzle commented Jun 18, 2014

jerone commented Jun 18, 2014

jerone commented Jun 22, 2014