Clean up river gauge #2011
Clean up river gauge #2011
Conversation
Yeah, I realized the |
root/inc/river-gauge.html
Outdated
| @@ -2,9 +2,9 @@ | |||
| Unlike an <img>, an <object> grafts the SVG document into the DOM, which | |||
| means browsers will display the <title> elements of the SVG. Yay! | |||
| --> | |||
| <object data="/river/gauge/<% distribution | uri | html %>" | |||
| <object data="/river/gauge/<% distribution | uri %>" | |||
There was a problem hiding this comment.
Relying on uri to do enough escaping for HTML too is bad practice. Why did you remove it here?
There was a problem hiding this comment.
I was checking on how the default filtering is applied and it slipped into the commit.
| @@ -48,8 +48,8 @@ | |||
| IF have_released; %> | |||
| <a class="latest" href="<% IF module %>/pod/<% module.documentation %><% ELSE %>/release/<% release.distribution; END %>" title="<%- IF release.maturity == 'developer'; 'dev release, '; END %>go to latest"><span class="fa fa-step-forward"></span></a> | |||
| <%- END; END; %> | |||
| <div class="inline"><% INCLUDE inc/river-gauge.html, distribution = release.distribution %></div> | |||
| <div class="inline"><%- INCLUDE inc/favorite.html %></div> | |||
There was a problem hiding this comment.
The reason for removing the inline class in an otherwise unrelated change isn't mentioned. From the face of it, it seems like it would break https://github.com/metacpan/metacpan-web/blob/d3f34a1/root/static/js/cpan.js#L301 (although the correctness and provenance of that line seems dubious at first glance).
There was a problem hiding this comment.
The div serves no real purpose, and then uses a silly class like "inline" to fix it. I'll fix the javascript to handle this properly.
There was a problem hiding this comment.
I totally agree it's a stupid bit of markup. :-)
|
@haarg Did you see #2011 (comment)? It looks like GitHub decided that was on an outdated commit for some reason and doesn't show it by default. |
haarg
force-pushed
the
haarg/river-cleanups
branch
2 times, most recently
from
da74305
to
1dbc249
Compare
|
Hrm. Switching to |
|
|
|
(Otherwise, this looks great and I appreciate the cleanups.) |
Various tweaks to the CSS and markup to make the river gauge layout more consistent across browsers. Also removes some needless extra divs in the breadcrumb that served no purpose. They were styled as inline, so they effectively functioned as spans, which without any extra styles apply effectively means they were no-ops. There was a bit of javascript relying on the "inline" class that was applied surrounding the favorite button, but it was just reusing classes that happened to be there rather than anything semantically tied to the button.
Using a newline entity in title seems to work properly in Firefox, Chrome, and Safari.
haarg
force-pushed
the
haarg/river-cleanups
branch
from
1dbc249
to
5795229
Compare
|
Merged! Thank you! |
Layout and some other cleanups for river gauge. It also embeds the svg directly on pod and release pages. We could also embed it on other pages if that was desired, but the data wasn't immediately available so I didn't both at this point.
The html br tags in the SVG don't work in Firefox, but an encoded newline seems to, so I replaced them.
Search page
Chrome before:
After:
Firefox before:
After:
Pod breadcrumbs
Chrome before:
After:
Firefox before:
After:
Release list
Chrome before:
After:
Firefox before:
After: