Issue 29471608: Issue 5043 - Update member page layout (Closed)
|
Created:
June 22, 2017, 6:08 p.m. by juliandoucette Modified:
July 23, 2017, 1:53 p.m. Reviewers:
ire Base URL:
https://hg.adblockplus.org/web.acceptableads.com Visibility:
Public. |
DescriptionIssue 5043 - Update member page layout
Patch Set 1 #Patch Set 2 : Removed accidental CSS change #
Total comments: 40
Patch Set 3 : Addressed comments #Patch Set 4 : Changed 3em member back to 4em #Patch Set 5 : Hard coded order of coalitions #
Total comments: 14
Patch Set 6 : Addressed comments #Patch Set 7 : Fixed capitilization and 0px #
Total comments: 4
Patch Set 8 : Fixed regression #MessagesTotal messages: 14
On 2017/06/22 18:08:42, juliandoucette wrote: Ready for review. Note: I had to deviate from the wireframes a little bit to make this responsive (e.g. 2 members per line on tablet, 3 on desktop) and consistent with the rest of the website (e.g. header font sizes). There is still a lot that I don't like about it (e.g. the image dimensions on tablet and many page unique styles) but the current state reflects where I chose to draw the line between consistency and this design.
@Ire do you want to have a look at this first so that we don't bother saroyanm. I'll let him have a last glance before it goes live.
On 2017/06/22 18:16:19, juliandoucette wrote: > @Ire do you want to have a look at this first so that we don't bother saroyanm. > I'll let him have a last glance before it goes live. Okay I will do.
Added some reflections (sorry if I'm doing this in the middle of your review!) https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... File pages/committee/members.tmpl (right): https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:1: title=Members | The Acceptable Ads Committee Note: I had originally used a schema on this page (an organization of members) but I decided to exclude these properties for this version in the interest of publishing this sooner - as we have not used schemas consistently throughout this website (and we probably should - as a separate issue). https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:7: <style> Note: I would have pushed back on the custom elements of this design - but the page was so bland without them that I thought it would be counter-productive (we would/will have to change a lot more across the whole website to make pages with this many heading levels look good) - and we want to get this online ASAP. https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:67: {% set committee = [ Note: I put this inside the template because... 1. Global python files are ascii by default (and I didn't know how to make them utf-8 at the time - I discussed this with kvas later in #websites) 2. The committee members are not currently being used in other templates - They may in the future when (if) we have individual committee member pages like we had originally planned - I doubt this will happen soon (if ever) https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:68: { Note: Please double check that I didn't miss anyone / publish someone that I shouldn't have. I... - Exported this as csv from a spreadsheet - Converted that csv to json based on the heading/columns - Ran a series manipulations based on expressions that I didn't document (e.g. combined the name, changed the keys, removed fields that do not display, removed candidates that are not final or announced) https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:312: <div class="col-6"> Note: I could have used a col-7, col-5 split for better image dimensions (like I mentioned in the ticket), but col-7/col-5 collapses sooner than col-6/col-6 and I didn't want that (or to change how columns work). https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:319: <img class="block" alt="{{ "Acceptable Ads Committee Structure" | translate("featured-image-alt", "Image alt text") }}" src="https://acceptableads.com/img/png/blog/acceptable-ads-committee-structure.png" /> NIT: I didn't need to add the protocol and domain here. https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:327: <h3 class="center p-y-md m-a-md">{{ "Meet the Acceptable Ads Committee" | translate("committee-heading", "heading") }}</h3> NIT: I don't like the p- m- classes here - but I didn't come up with a better solution that is consistent with the rest of the site. Any ideas? https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:332: <h2>{{ "For-Profit Coalition" | translate("for-profit-heading", "heading") }}</h2> Note: I could also put this header, paragraph, set of lists into a macro.
You taught me about Jinja macros today :) 👍🏾 https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... File pages/committee/members.tmpl (right): https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:1: title=Members | The Acceptable Ads Committee On 2017/06/23 12:55:21, juliandoucette wrote: > Note: I had originally used a schema on this page (an organization of members) > but I decided to exclude these properties for this version in the interest of > publishing this sooner - as we have not used schemas consistently throughout > this website (and we probably should - as a separate issue). Acknowledged. https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:7: <style> On 2017/06/23 12:55:21, juliandoucette wrote: > Note: I would have pushed back on the custom elements of this design - but the > page was so bland without them that I thought it would be counter-productive (we > would/will have to change a lot more across the whole website to make pages with > this many heading levels look good) - and we want to get this online ASAP. Acknowledged. https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:34: .member-list li The list items look a bit squashed together on mobile because they lose the `min-height: 4em` style. Could we have some spacing between each list item for smaller screens? https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:36: display: block; Since this element is floated, the `display: block` isn't making any difference https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:42: @media(min-width: 767px) The $tablet-breakpoint is 768px https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:62: content: none; Setting `content: none` doesn't seem to be doing anything additional here https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:67: {% set committee = [ On 2017/06/23 12:55:21, juliandoucette wrote: > Note: I put this inside the template because... > > 1. Global python files are ascii by default (and I didn't know how to make them > utf-8 at the time - I discussed this with kvas later in #websites) > 2. The committee members are not currently being used in other templates > - They may in the future when (if) we have individual committee member pages > like we had originally planned > - I doubt this will happen soon (if ever) Acknowledged. https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:68: { On 2017/06/23 12:55:21, juliandoucette wrote: > Note: Please double check that I didn't miss anyone / publish someone that I > shouldn't have. I... > > - Exported this as csv from a spreadsheet > - Converted that csv to json based on the heading/columns > - Ran a series manipulations based on expressions that I didn't document (e.g. > combined the name, changed the keys, removed fields that do not display, removed > candidates that are not final or announced) Done. Everything looks correct to me. https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:209: "group": "Creative Agent", Sometimes the group is written in plural form, e.g. "Ad Tech Agencies" and sometimes it's singular like "Creative Agent" or "Researcher". We should stick to one form (preferably plural) https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:282: {% for member in committee %} Should we have some fallback message for if there are no members? Currently I think it looks a bit weird when there is just nothing there. Particularly for the "Users" section, where there is no Representative either. https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:312: <div class="col-6"> On 2017/06/23 12:55:21, juliandoucette wrote: > Note: I could have used a col-7, col-5 split for better image dimensions (like I > mentioned in the ticket), but col-7/col-5 collapses sooner than col-6/col-6 and > I didn't want that (or to change how columns work). Acknowledged. https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:319: <img class="block" alt="{{ "Acceptable Ads Committee Structure" | translate("featured-image-alt", "Image alt text") }}" src="https://acceptableads.com/img/png/blog/acceptable-ads-committee-structure.png" /> On 2017/06/23 12:55:21, juliandoucette wrote: > NIT: I didn't need to add the protocol and domain here. Was about to say :p https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:327: <h3 class="center p-y-md m-a-md">{{ "Meet the Acceptable Ads Committee" | translate("committee-heading", "heading") }}</h3> It seems like this should be an <h2> and the other headings below, e.g. "For-Profit Coalition", should be an <h3>? https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:327: <h3 class="center p-y-md m-a-md">{{ "Meet the Acceptable Ads Committee" | translate("committee-heading", "heading") }}</h3> On 2017/06/23 12:55:22, juliandoucette wrote: > NIT: I don't like the p- m- classes here - but I didn't come up with a better > solution that is consistent with the rest of the site. Any ideas? I see what you mean but I don't think they are actually a problem here. The spacing on this element seems alright to me. I think it's the spacing on the nearby elements that look too big on mobile, e.g. the .section and h2, and if I remember correctly they will be fixed by #5135 https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:332: <h2>{{ "For-Profit Coalition" | translate("for-profit-heading", "heading") }}</h2> On 2017/06/23 12:55:21, juliandoucette wrote: > Note: I could also put this header, paragraph, set of lists into a macro. Yes I think you should. https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:333: <p class="col-6">{{ "This Coalition consists of stakeholders that can be classified as organizations primarily driven by generating profits. The following groups form the For-Profit Coalition: Advertisers, Ad-Tech Agencies, Advertising Agencies, and Publishers and Content Creators." | translate("for-profit-intro") }}</p> Why does only this paragraph have a `.col-6`? https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:347: <h2>{{ "User ADvocate Coalition" | translate("user-advocate-coalition", "heading") }}</h2> NIT: "Advocate", not "ADvocate"
Ready for review. https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... File pages/committee/members.tmpl (right): https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:34: .member-list li On 2017/06/25 19:31:10, ire wrote: > The list items look a bit squashed together on mobile because they lose the > `min-height: 4em` style. Could we have some spacing between each list item for > smaller screens? Done. Good catch :) https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:36: display: block; On 2017/06/25 19:31:10, ire wrote: > Since this element is floated, the `display: block` isn't making any difference Done. Verified; I did not know this :O https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:42: @media(min-width: 767px) On 2017/06/25 19:31:10, ire wrote: > The $tablet-breakpoint is 768px We usually -1 when we use max-width NOT min-width (sorry - my mistake) https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:42: @media(min-width: 767px) On 2017/06/25 19:31:10, ire wrote: > The $tablet-breakpoint is 768px Done. https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:62: content: none; On 2017/06/25 19:31:10, ire wrote: > Setting `content: none` doesn't seem to be doing anything additional here Done. It was doing something - just not something you can see (I'm making excuses - thank you for pointing this out). https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:68: { On 2017/06/25 19:31:09, ire wrote: > On 2017/06/23 12:55:21, juliandoucette wrote: > > Note: Please double check that I didn't miss anyone / publish someone that I > > shouldn't have. I... > > > > - Exported this as csv from a spreadsheet > > - Converted that csv to json based on the heading/columns > > - Ran a series manipulations based on expressions that I didn't document (e.g. > > combined the name, changed the keys, removed fields that do not display, > removed > > candidates that are not final or announced) > > Done. Everything looks correct to me. Acknowledged. https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:209: "group": "Creative Agent", On 2017/06/25 19:31:10, ire wrote: > Sometimes the group is written in plural form, e.g. "Ad Tech Agencies" and > sometimes it's singular like "Creative Agent" or "Researcher". We should stick > to one form (preferably plural) Acknowledged. I think these names don't match the ticket. I will match them. ~Good catch. https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:209: "group": "Creative Agent", On 2017/06/25 19:31:10, ire wrote: > Sometimes the group is written in plural form, e.g. "Ad Tech Agencies" and > sometimes it's singular like "Creative Agent" or "Researcher". We should stick > to one form (preferably plural) Done. https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:282: {% for member in committee %} On 2017/06/25 19:31:09, ire wrote: > Should we have some fallback message for if there are no members? Currently I > think it looks a bit weird when there is just nothing there. Particularly for > the "Users" section, where there is no Representative either. Done. https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:319: <img class="block" alt="{{ "Acceptable Ads Committee Structure" | translate("featured-image-alt", "Image alt text") }}" src="https://acceptableads.com/img/png/blog/acceptable-ads-committee-structure.png" /> On 2017/06/23 12:55:21, juliandoucette wrote: > NIT: I didn't need to add the protocol and domain here. Done. https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:327: <h3 class="center p-y-md m-a-md">{{ "Meet the Acceptable Ads Committee" | translate("committee-heading", "heading") }}</h3> On 2017/06/25 19:31:09, ire wrote: > It seems like this should be an <h2> and the other headings below, e.g. > "For-Profit Coalition", should be an <h3>? Yes. Unfortunately the semantically correct header levels do not match the intended style. Done. https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:332: <h2>{{ "For-Profit Coalition" | translate("for-profit-heading", "heading") }}</h2> On 2017/06/25 19:31:10, ire wrote: > On 2017/06/23 12:55:21, juliandoucette wrote: > > Note: I could also put this header, paragraph, set of lists into a macro. > > Yes I think you should. Done. https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:333: <p class="col-6">{{ "This Coalition consists of stakeholders that can be classified as organizations primarily driven by generating profits. The following groups form the For-Profit Coalition: Advertisers, Ad-Tech Agencies, Advertising Agencies, and Publishers and Content Creators." | translate("for-profit-intro") }}</p> On 2017/06/25 19:31:09, ire wrote: > Why does only this paragraph have a `.col-6`? They should all be. The design specifies 1/2 width. https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:347: <h2>{{ "User ADvocate Coalition" | translate("user-advocate-coalition", "heading") }}</h2> On 2017/06/25 19:31:09, ire wrote: > NIT: "Advocate", not "ADvocate" Not a NIT :D - Good catch. https://codereview.adblockplus.org/29471608/diff/29471621/pages/committee/mem... pages/committee/members.tmpl:347: <h2>{{ "User ADvocate Coalition" | translate("user-advocate-coalition", "heading") }}</h2> On 2017/06/25 19:31:09, ire wrote: > NIT: "Advocate", not "ADvocate" Done.
https://codereview.adblockplus.org/29471608/diff/29474638/pages/committee/mem... File pages/committee/members.tmpl (right): https://codereview.adblockplus.org/29471608/diff/29474638/pages/committee/mem... pages/committee/members.tmpl:54: padding-left: 0px; Unit not needed https://codereview.adblockplus.org/29471608/diff/29474638/pages/committee/mem... pages/committee/members.tmpl:84: "title": "FOR-PROFIT COALITION", NIT: I prefer the capitalization to be applied with CSS https://codereview.adblockplus.org/29471608/diff/29474638/pages/committee/mem... pages/committee/members.tmpl:335: {{ member.title }}, <em>{{ member.org }}</em> Should job titles be translated? https://codereview.adblockplus.org/29471608/diff/29474638/pages/committee/mem... pages/committee/members.tmpl:349: <h4>{{ group | translate("group-name-heading", "heading") }}</h4> It seems like this "group-name-heading" ID will be applied to multiple different strings https://codereview.adblockplus.org/29471608/diff/29474638/pages/committee/mem... pages/committee/members.tmpl:376: <p>{{ "The Acceptable Ads Committee is divided into three Coalitions, with each faction consisting of several Members. Each group includes a Representative and multiple supporting Members." | translate("members-intro-2") }}</p> Translation ID should be "members-intro-1"
@Jeen, @Tamara Please answer the following question [here](https://codereview.adblockplus.org/29471608/diff/29474638/pages/committee/members.tmpl#newcode335) Should we eventually translate the job titles of our committee members? I don't think so. But it's worth asking. https://codereview.adblockplus.org/29471608/diff/29474638/pages/committee/mem... File pages/committee/members.tmpl (right): https://codereview.adblockplus.org/29471608/diff/29474638/pages/committee/mem... pages/committee/members.tmpl:54: padding-left: 0px; On 2017/07/06 15:41:21, ire wrote: > Unit not needed Acknowledged. I agree. But saroyanm and greiner fought me on this one. I think our last conclusion was to add units. https://codereview.adblockplus.org/29471608/diff/29474638/pages/committee/mem... pages/committee/members.tmpl:84: "title": "FOR-PROFIT COALITION", On 2017/07/06 15:41:21, ire wrote: > NIT: I prefer the capitalization to be applied with CSS Acknowledged. I agree. But I copied this content from elsewhere - and didn't think it was necessary to manually change - considering I can un-capitalize it with CSS too. Do you know how to do this automatically in a text editor? https://codereview.adblockplus.org/29471608/diff/29474638/pages/committee/mem... pages/committee/members.tmpl:335: {{ member.title }}, <em>{{ member.org }}</em> On 2017/07/06 15:41:21, ire wrote: > Should job titles be translated? I don't think so. @Jeen? @Tamara? https://codereview.adblockplus.org/29471608/diff/29474638/pages/committee/mem... pages/committee/members.tmpl:349: <h4>{{ group | translate("group-name-heading", "heading") }}</h4> On 2017/07/06 15:41:21, ire wrote: > It seems like this "group-name-heading" ID will be applied to multiple different > strings Done. Good catch. https://codereview.adblockplus.org/29471608/diff/29474638/pages/committee/mem... pages/committee/members.tmpl:376: <p>{{ "The Acceptable Ads Committee is divided into three Coalitions, with each faction consisting of several Members. Each group includes a Representative and multiple supporting Members." | translate("members-intro-2") }}</p> On 2017/07/06 15:41:21, ire wrote: > Translation ID should be "members-intro-1" Done. Good catch.
Thanks :) https://codereview.adblockplus.org/29471608/diff/29474638/pages/committee/mem... File pages/committee/members.tmpl (right): https://codereview.adblockplus.org/29471608/diff/29474638/pages/committee/mem... pages/committee/members.tmpl:54: padding-left: 0px; On 2017/07/07 14:20:00, juliandoucette wrote: > On 2017/07/06 15:41:21, ire wrote: > > Unit not needed > > Acknowledged. > > I agree. But saroyanm and greiner fought me on this one. I think our last > conclusion was to add units. Acknowledged. But does that mean units should be applied everywhere there's a 0? i.e. elsewhere in this file. https://codereview.adblockplus.org/29471608/diff/29474638/pages/committee/mem... pages/committee/members.tmpl:84: "title": "FOR-PROFIT COALITION", On 2017/07/07 14:20:00, juliandoucette wrote: > On 2017/07/06 15:41:21, ire wrote: > > NIT: I prefer the capitalization to be applied with CSS > > Acknowledged. > > I agree. But I copied this content from elsewhere - and didn't think it was > necessary to manually change - considering I can un-capitalize it with CSS too. > Do you know how to do this automatically in a text editor? Okay. I don't know of a way to do it in a text editor, but I found this online - http://titlecase.com/
https://codereview.adblockplus.org/29471608/diff/29474638/pages/committee/mem... File pages/committee/members.tmpl (right): https://codereview.adblockplus.org/29471608/diff/29474638/pages/committee/mem... pages/committee/members.tmpl:54: padding-left: 0px; On 2017/07/10 07:43:00, ire wrote: > On 2017/07/07 14:20:00, juliandoucette wrote: > > On 2017/07/06 15:41:21, ire wrote: > > > Unit not needed > > > > Acknowledged. > > > > I agree. But saroyanm and greiner fought me on this one. I think our last > > conclusion was to add units. > > Acknowledged. But does that mean units should be applied everywhere there's a 0? > i.e. elsewhere in this file. Done. https://codereview.adblockplus.org/29471608/diff/29474638/pages/committee/mem... pages/committee/members.tmpl:84: "title": "FOR-PROFIT COALITION", On 2017/07/10 07:43:00, ire wrote: > Okay. I don't know of a way to do it in a text editor, but I found this online - > http://titlecase.com/ Done. I'm sure there is a better way, but I ended up using emacs' M-x capitalize-region.
It looks like some of the changes you made previously were undone. See comments below https://codereview.adblockplus.org/29471608/diff/29489569/pages/committee/mem... File pages/committee/members.tmpl (right): https://codereview.adblockplus.org/29471608/diff/29489569/pages/committee/mem... pages/committee/members.tmpl:404: <h4>{{ group | translate("group-name-heading", "heading") }}</h4> This is back to not using an ID https://codereview.adblockplus.org/29471608/diff/29489569/pages/committee/mem... pages/committee/members.tmpl:431: <p>{{ "The Acceptable Ads Committee is divided into three Coalitions, with each faction consisting of several Members. Each group includes a Representative and multiple supporting Members." | translate("members-intro-2") }}</p> This should be members-intro-1
Thanks / Sorry :) https://codereview.adblockplus.org/29471608/diff/29489569/pages/committee/mem... File pages/committee/members.tmpl (right): https://codereview.adblockplus.org/29471608/diff/29489569/pages/committee/mem... pages/committee/members.tmpl:404: <h4>{{ group | translate("group-name-heading", "heading") }}</h4> On 2017/07/17 07:27:49, ire wrote: > This is back to not using an ID Done. https://codereview.adblockplus.org/29471608/diff/29489569/pages/committee/mem... pages/committee/members.tmpl:431: <p>{{ "The Acceptable Ads Committee is divided into three Coalitions, with each faction consisting of several Members. Each group includes a Representative and multiple supporting Members." | translate("members-intro-2") }}</p> On 2017/07/17 07:27:49, ire wrote: > This should be members-intro-1 Done.
On 2017/07/17 10:37:40, juliandoucette wrote: > Thanks / Sorry :) No worries. LGTM :) 
|
