Issue 29337807: Issue 3097 - Refactored subscriptions list table on adblockplus.org/en/subscriptions

Issue 29337807: Issue 3097 - Refactored subscriptions list table on adblockplus.org/en/subscriptions (Closed)

Created:
Feb. 27, 2016, 4:16 p.m. by juliandoucette

Modified:
Aug. 16, 2016, 4:56 p.m.

Reviewers:
Thomas Greiner, saroyanm

CC:
kzar

Visibility:
Public.

Description

Changed subscriptions list table into a hierarchical list for accessibility and to support multi-level supplemented lists.

Patch Set 1 : Changed table into unordered list, refactored list markup, refactored main css and templates to support multiple footer elements #

Total comments: 15

Patch Set 2 : Refactored markup & styles, added rtl styles, and fixed typo. #

Total comments: 10

Patch Set 3 : Changed footer-main class to id, removed .screen-reader-text class #

Total comments: 2

Patch Set 4 : Removed extra whitespace #

Created: July 29, 2016, 10:52 a.m.

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+159 lines, -83 lines)			Patch
M	includes/subscriptionList.tmpl	View	1 2 3	1 chunk	+67 lines, -49 lines	0 comments	Download
M	pages/subscriptions.tmpl	View	1 2	1 chunk	+69 lines, -12 lines	0 comments	Download
M	static/css/main.css	View	1 2	1 chunk	+5 lines, -4 lines	0 comments	Download
M	static/css/main-desktop.css	View	1 2	1 chunk	+8 lines, -8 lines	0 comments	Download
M	static/css/main-mobile.css	View	1 2	2 chunks	+8 lines, -8 lines	0 comments	Download
M	templates/default.tmpl	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	templates/simple.tmpl	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download

Messages

Total messages: 10

Expand All Messages | Collapse All Messages

Thomas Greiner

https://codereview.adblockplus.org/29337807/diff/29337808/includes/subscriptionList.tmpl File includes/subscriptionList.tmpl (right): https://codereview.adblockplus.org/29337807/diff/29337808/includes/subscriptionList.tmpl#newcode25 includes/subscriptionList.tmpl:25: <span class="sr-only">{{ "Specialization"|translate("specialization") }}:</span> Detail: Not sure what "sr-only" ...

July 15, 2016, 4:07 p.m. (2016-07-15 16:07:55 UTC) #2

juliandoucette

Patch submitted. https://codereview.adblockplus.org/29337807/diff/29337808/includes/subscriptionList.tmpl File includes/subscriptionList.tmpl (right): https://codereview.adblockplus.org/29337807/diff/29337808/includes/subscriptionList.tmpl#newcode25 includes/subscriptionList.tmpl:25: <span class="sr-only">{{ "Specialization"|translate("specialization") }}:</span> On 2016/07/15 16:07:53, ...

July 19, 2016, 10:29 p.m. (2016-07-19 22:29:18 UTC) #3

Patch submitted.

https://codereview.adblockplus.org/29337807/diff/29337808/includes/subscripti...
File includes/subscriptionList.tmpl (right):

https://codereview.adblockplus.org/29337807/diff/29337808/includes/subscripti...
includes/subscriptionList.tmpl:25: <span class="sr-only">{{
"Specialization"|translate("specialization") }}:</span>
On 2016/07/15 16:07:53, Thomas Greiner wrote:
> Detail: Not sure what "sr-only" means but it's not being used for styling so
it
> looks like you can remove the `<span>` around the text entirely.

My mistake.

"sr" stands for "screen reader".

I forgot to add styles for this class.

https://codereview.adblockplus.org/29337807/diff/29337808/includes/subscripti...
includes/subscriptionList.tmpl:29: <p>{{ "supplement
for"|translate("supplements_prefix") }} {{ ' /
'.join(subscription["supplements"]) }} {{ " "|translate("supplements_suffix")
}}.</p>
On 2016/07/15 16:07:53, Thomas Greiner wrote:
> Detail: Mind splitting this content (and the content from other paragraphs) up
> into multiple code lines to make it easier to read? Because I noticed you did
> that in the one above and it shouldn't have any impact on how the text is
> displayed on the page.

Done.

https://codereview.adblockplus.org/29337807/diff/29337808/includes/subscripti...
includes/subscriptionList.tmpl:34: <div class="subscription-authors">
On 2016/07/15 16:07:53, Thomas Greiner wrote:
> Detail: Since you've decided to use `<footer>` for containing the filter list
> meta data, what about using `<address>` instead of this `<div>` here?
> 
> See https://developer.mozilla.org/en-US/docs/Web/HTML/Element/address

Good idea.

https://codereview.adblockplus.org/29337807/diff/29337808/pages/subscriptions...
File pages/subscriptions.tmpl (right):

https://codereview.adblockplus.org/29337807/diff/29337808/pages/subscriptions...
pages/subscriptions.tmpl:24: margin: 0;
On 2016/07/15 16:07:54, Thomas Greiner wrote:
> Coding style: "CSS number values should specify units where possible."
> 
> Same applies to other parts of the code.

Done.

https://codereview.adblockplus.org/29337807/diff/29337808/pages/subscriptions...
pages/subscriptions.tmpl:35: padding-left: 30px;
On 2016/07/15 16:07:54, Thomas Greiner wrote:
> Please avoid using "border/margin/padding-bottom/left/right/top" and "float"
> because it messes with the layout for right-to-left languages.
> 
> Note that, unlike with the current page, the alignment of the elements looks a
> bit off when setting `<html dir="rtl">` so it'd be great if we could prevent
> that sort of regression.
> 
> For styles that you can't do otherwise you can create additional rules
prefixed
> with `html[dir="rtl"]`.

I hadn't considered rtl when I created this patch.

I will account for it in the next.

https://codereview.adblockplus.org/29337807/diff/29337808/pages/subscriptions...
pages/subscriptions.tmpl:43: and missing connections in hirarchy */
On 2016/07/15 16:07:54, Thomas Greiner wrote:
> Typo: Replace "hirarchy" with "hierarchy".

Thanks! :)

https://codereview.adblockplus.org/29337807/diff/29337808/static/css/main.css
File static/css/main.css (left):

https://codereview.adblockplus.org/29337807/diff/29337808/static/css/main.css...
static/css/main.css:256: #content h1
On 2016/07/15 16:07:54, Thomas Greiner wrote:
> Did you make sure that this won't have any side effects on any other pages?

Yes. But I will revert this main.css change and apply a more specific style to
the page instead anyway. I don't know what I was thinking here.

Thomas Greiner

https://codereview.adblockplus.org/29337807/diff/29337808/static/css/main.css File static/css/main.css (left): https://codereview.adblockplus.org/29337807/diff/29337808/static/css/main.css#oldcode256 static/css/main.css:256: #content h1 On 2016/07/19 22:29:17, juliandoucette wrote: > On ...

July 27, 2016, 9:42 a.m. (2016-07-27 09:42:10 UTC) #4

juliandoucette

One question in the comments. https://codereview.adblockplus.org/29337807/diff/29348048/pages/subscriptions.tmpl File pages/subscriptions.tmpl (right): https://codereview.adblockplus.org/29337807/diff/29348048/pages/subscriptions.tmpl#newcode23 pages/subscriptions.tmpl:23: clip: rect(1px, 1px, 1px, ...

July 27, 2016, 12:11 p.m. (2016-07-27 12:11:01 UTC) #5

Thomas Greiner

https://codereview.adblockplus.org/29337807/diff/29348048/pages/subscriptions.tmpl File pages/subscriptions.tmpl (right): https://codereview.adblockplus.org/29337807/diff/29348048/pages/subscriptions.tmpl#newcode23 pages/subscriptions.tmpl:23: clip: rect(1px, 1px, 1px, 1px); On 2016/07/27 12:11:01, juliandoucette ...

July 27, 2016, 1:25 p.m. (2016-07-27 13:25:49 UTC) #6

juliandoucette

July 28, 2016, 9:34 p.m. (2016-07-28 21:34:02 UTC) #7

https://codereview.adblockplus.org/29337807/diff/29348048/pages/subscriptions...
File pages/subscriptions.tmpl (right):

https://codereview.adblockplus.org/29337807/diff/29348048/pages/subscriptions...
pages/subscriptions.tmpl:23: clip: rect(1px, 1px, 1px, 1px);
On 2016/07/27 13:25:48, Thomas Greiner wrote:
> On 2016/07/27 12:11:01, juliandoucette wrote:
> > On 2016/07/27 09:42:10, Thomas Greiner wrote:
> > > According to https://developer.mozilla.org/en-US/docs/Web/CSS/clip this
> > property
> > > has been deprecated so it'd be great if we could avoid using it.
> > > 
> > > I'm just wondering why we need special treatment for this element in the
> first
> > > place. If it's helpful for visually impaired visitors it's probably also
> good
> > > for the rest, right?
> > > 
> > > I'm ignoring the fact that this text doesn't exist in the current version
> and
> > > hasn't been mentioned in the ticket either.
> > 
> > I agree. 
> > 
> > I vote: Leave the text and remove the class + definition.
> > 
> > Is that ok with you?
> 
> Yep, sounds good. :)

Done.

https://codereview.adblockplus.org/29337807/diff/29348048/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29337807/diff/29348048/templates/default.t...
templates/default.tmpl:146: <footer class="footer-main">
On 2016/07/27 13:25:49, Thomas Greiner wrote:
> On 2016/07/27 12:11:01, juliandoucette wrote:
> > On 2016/07/27 09:42:10, Thomas Greiner wrote:
> > > Detail: What about using an ID instead? Because it doesn't look like we'll
> > have
> > > other elements like this one on the page.
> > 
> > I did that habitually based on csslint rules
> >
>
http://stackoverflow.com/questions/8084555/why-selecting-by-id-is-not-recomme...
> > 
> > I should probably use an ID to be more consistent with the way the rest of
our
> > CSS is written.
> 
> It's a discussion that we probably need to have in #4155. But yeah, until then
> best to keep things consistent.

Done.

Thomas Greiner

Apart from the remaining coding style issue: LGTM https://codereview.adblockplus.org/29337807/diff/29348833/includes/subscriptionList.tmpl File includes/subscriptionList.tmpl (right): https://codereview.adblockplus.org/29337807/diff/29348833/includes/subscriptionList.tmpl#newcode25 includes/subscriptionList.tmpl:25: {{ ...

July 29, 2016, 10:07 a.m. (2016-07-29 10:07:39 UTC) #8

juliandoucette

https://codereview.adblockplus.org/29337807/diff/29348833/includes/subscriptionList.tmpl File includes/subscriptionList.tmpl (right): https://codereview.adblockplus.org/29337807/diff/29348833/includes/subscriptionList.tmpl#newcode25 includes/subscriptionList.tmpl:25: {{ "Specialization"|translate("specialization") }}: On 2016/07/29 10:07:38, Thomas Greiner wrote: ...

July 29, 2016, 11:01 a.m. (2016-07-29 11:01:36 UTC) #9

juliandoucette

Aug. 16, 2016, 4:56 p.m. (2016-08-16 16:56:01 UTC) #10

https://hg.adblockplus.org/web.adblockplus.org/rev/139f9531dc3c

Expand All Messages | Collapse All Messages