Issue 29464633: No Issue - Help Center Development Breakdown (Closed)
|
Created:
June 13, 2017, 2:35 p.m. by ire Modified:
June 21, 2017, 8:35 a.m. Reviewers:
juliandoucette Visibility:
Public. |
DescriptionNo Issue - Help Center Development Breakdown
Patch Set 1 #
Total comments: 17
Patch Set 2 : Remove home template, add default template, separate header and footer #
Total comments: 4
Patch Set 3 : Remove Search results page and template #MessagesTotal messages: 6
Here's an overview of the pages, templates, and components. Let me know if I should go in more detail on anything. https://codereview.adblockplus.org/29464633/diff/29464634/breakdown.md File breakdown.md (right): https://codereview.adblockplus.org/29464633/diff/29464634/breakdown.md#newcode8 breakdown.md:8: - Home This page isn't part of the current version
Thanks Ire! https://codereview.adblockplus.org/29464633/diff/29464634/breakdown.md File breakdown.md (right): https://codereview.adblockplus.org/29464633/diff/29464634/breakdown.md#newcode8 breakdown.md:8: - Home On 2017/06/13 14:37:49, ire wrote: > This page isn't part of the current version We can remove this from the specification of this version. https://codereview.adblockplus.org/29464633/diff/29464634/breakdown.md#newcode17 breakdown.md:17: - Help Center Home See comment above. https://codereview.adblockplus.org/29464633/diff/29464634/breakdown.md#newcode21 breakdown.md:21: - Basic Page - This can be the default template - Other templates can extend this one (http://jinja.pocoo.org/docs/2.9/templates/#child-template) https://codereview.adblockplus.org/29464633/diff/29464634/breakdown.md#newcode27 breakdown.md:27: - Site Search Bar NIT: This is a field not a bar. https://codereview.adblockplus.org/29464633/diff/29464634/breakdown.md#newcode30 breakdown.md:30: - Breadcrumb NIT: Breadcrumb**s** or Breadcrumb bar? https://codereview.adblockplus.org/29464633/diff/29464634/breakdown.md#newcode35 breakdown.md:35: - Lists (Ordered Lists, Unordered Lists, "Numbered" Lists) It's not clear to me whether there should also be "*Underlined* Lists" (The Topic Group, Accordion Menu, and Search Result lists are quite similar in style). https://codereview.adblockplus.org/29464633/diff/29464634/breakdown.md#newcode46 breakdown.md:46: This includes: Why did you group the header navbar and the footer navbar into the same component? I think they are separate includes. And, by extension, separate components. But this may just be a misunderstanding of what we mean by "component" here. https://codereview.adblockplus.org/29464633/diff/29464634/breakdown.md#newcode52 breakdown.md:52: Note: The header has a slighlty different layout depening on if it is within a specific product's page (e.g. Single Article) or a general page (e.g. Basic Page) Thanks for pointing this out.
https://codereview.adblockplus.org/29464633/diff/29464634/breakdown.md File breakdown.md (right): https://codereview.adblockplus.org/29464633/diff/29464634/breakdown.md#newcode8 breakdown.md:8: - Home On 2017/06/16 16:21:31, juliandoucette wrote: > On 2017/06/13 14:37:49, ire wrote: > > This page isn't part of the current version > > We can remove this from the specification of this version. Acknowledged. https://codereview.adblockplus.org/29464633/diff/29464634/breakdown.md#newcode17 breakdown.md:17: - Help Center Home On 2017/06/16 16:21:32, juliandoucette wrote: > See comment above. Acknowledged. https://codereview.adblockplus.org/29464633/diff/29464634/breakdown.md#newcode21 breakdown.md:21: - Basic Page On 2017/06/16 16:21:31, juliandoucette wrote: > - This can be the default template > - Other templates can extend this one > (http://jinja.pocoo.org/docs/2.9/templates/#child-template) Yes we should have a default template, but here I was referring to a template for a basic page, like the Imprint (I should probably use a different name as it's confusing). This has a slightly different layout to other pages, so it should itself extend from the default template we create https://codereview.adblockplus.org/29464633/diff/29464634/breakdown.md#newcode27 breakdown.md:27: - Site Search Bar On 2017/06/16 16:21:31, juliandoucette wrote: > NIT: This is a field not a bar. Acknowledged. https://codereview.adblockplus.org/29464633/diff/29464634/breakdown.md#newcode30 breakdown.md:30: - Breadcrumb On 2017/06/16 16:21:32, juliandoucette wrote: > NIT: Breadcrumb**s** or Breadcrumb bar? I think "Breadcrumbs" https://codereview.adblockplus.org/29464633/diff/29464634/breakdown.md#newcode35 breakdown.md:35: - Lists (Ordered Lists, Unordered Lists, "Numbered" Lists) On 2017/06/16 16:21:31, juliandoucette wrote: > It's not clear to me whether there should also be "*Underlined* Lists" (The > Topic Group, Accordion Menu, and Search Result lists are quite similar in > style). Yes you're right there's definitely a pattern there. I'll add that. https://codereview.adblockplus.org/29464633/diff/29464634/breakdown.md#newcode46 breakdown.md:46: This includes: On 2017/06/16 16:21:31, juliandoucette wrote: > Why did you group the header navbar and the footer navbar into the same > component? I think they are separate includes. And, by extension, separate > components. But this may just be a misunderstanding of what we mean by > "component" here. Yeah they will be separate includes. I was mostly following the way it was defined in the specification, but I'll separate them. https://codereview.adblockplus.org/29464633/diff/29464634/breakdown.md#newcode52 breakdown.md:52: Note: The header has a slighlty different layout depening on if it is within a specific product's page (e.g. Single Article) or a general page (e.g. Basic Page) On 2017/06/16 16:21:32, juliandoucette wrote: > Thanks for pointing this out. You're welcome :)
Minor issues left - but I think you get the idea - LGTM. Time to create issues for, implement, and review all of these. https://codereview.adblockplus.org/29464633/diff/29468569/breakdown.md File breakdown.md (right): https://codereview.adblockplus.org/29464633/diff/29468569/breakdown.md#newcode10 breakdown.md:10: - Search Results Page This page will not be in the MVP (Sorry, I forgot last time). https://codereview.adblockplus.org/29464633/diff/29468569/breakdown.md#newcode36 breakdown.md:36: NIT: It's not clear why you are separating these lists. I'm guessing you mean something like "Site specific components" and "Website default components".
https://codereview.adblockplus.org/29464633/diff/29468569/breakdown.md File breakdown.md (right): https://codereview.adblockplus.org/29464633/diff/29468569/breakdown.md#newcode10 breakdown.md:10: - Search Results Page On 2017/06/20 15:53:39, juliandoucette wrote: > This page will not be in the MVP (Sorry, I forgot last time). Acknowledged. https://codereview.adblockplus.org/29464633/diff/29468569/breakdown.md#newcode36 breakdown.md:36: On 2017/06/20 15:53:39, juliandoucette wrote: > NIT: It's not clear why you are separating these lists. I'm guessing you mean > something like "Site specific components" and "Website default components". Yeah, it's one of those things that just made more sense in my head to separate :P 
|
