Issue 29575597: Issue 5691 - Create Breadcrumbs Component for help.eyeo.com (Closed)
|
Created:
Oct. 13, 2017, 10:27 a.m. by ire Modified:
Oct. 25, 2017, 1:32 p.m. Reviewers:
juliandoucette Base URL:
https://hg.adblockplus.org/help.eyeo.com Visibility:
Public. |
DescriptionIssue 5691 - Create Breadcrumbs Component for help.eyeo.com
Patch Set 1 #
Total comments: 34
Patch Set 2 : Address NITs #
Total comments: 1
Patch Set 3 : Update link color #
Total comments: 11
Patch Set 4 : Addressed NITs #
MessagesTotal messages: 10
Ready for review https://codereview.adblockplus.org/29575597/diff/29575598/globals/products.py File globals/products.py (right): https://codereview.adblockplus.org/29575597/diff/29575598/globals/products.py... globals/products.py:20: "slug": "adblockplus", Adding this was the only way I could think of to access the product slug from the article page. Is there another way to get the parent page?
Stay awesome. https://codereview.adblockplus.org/29575597/diff/29575598/globals/products.py File globals/products.py (right): https://codereview.adblockplus.org/29575597/diff/29575598/globals/products.py... globals/products.py:20: "slug": "adblockplus", On 2017/10/13 10:30:00, ire wrote: > Adding this was the only way I could think of to access the product slug from > the article page. Is there another way to get the parent page? Hmm... The product key and slug could be the same. But I'm ok with the way that you have implemented this. https://codereview.adblockplus.org/29575597/diff/29575598/static/scss/compone... File static/scss/components/_breadcrumbs.scss (right): https://codereview.adblockplus.org/29575597/diff/29575598/static/scss/compone... static/scss/components/_breadcrumbs.scss:19: padding-top: $md; NIT/Suggest: 1em? https://codereview.adblockplus.org/29575597/diff/29575598/static/scss/compone... static/scss/components/_breadcrumbs.scss:27: display: inline-block; NIT: This will cause a whitespace between list items that will scale with the font-size. I would use the floating box model instead. https://codereview.adblockplus.org/29575597/diff/29575598/static/scss/compone... static/scss/components/_breadcrumbs.scss:30: color: $gray-dark; NIT: Isn't the blue link supposed to be grey too? https://codereview.adblockplus.org/29575597/diff/29575598/static/scss/compone... static/scss/components/_breadcrumbs.scss:32: font-weight: $bold-weight; This is semibold in the mockups & styleguide https://codereview.adblockplus.org/29575597/diff/29575598/static/scss/compone... static/scss/components/_breadcrumbs.scss:38: padding-right: 0; SuperNIT: shorthand padding causes less characters (we should decide which way to go in our coding style if we haven't already - not that it matters very much) PS: "SuperNIT" (which I made up?) means less than a NIT not more than a NIT https://codereview.adblockplus.org/29575597/diff/29575598/static/scss/compone... static/scss/components/_breadcrumbs.scss:39: padding-left: $lg; NIT/Suggest: 2em? https://codereview.adblockplus.org/29575597/diff/29575598/static/scss/compone... static/scss/components/_breadcrumbs.scss:45: top: 0.25em; NIT: This looks slightly too low Suggest: explain this value via comment https://codereview.adblockplus.org/29575597/diff/29575598/static/scss/compone... static/scss/components/_breadcrumbs.scss:46: right: $lg / 4; NIT/Suggest: Explain this via comment. https://codereview.adblockplus.org/29575597/diff/29575598/static/scss/compone... static/scss/components/_breadcrumbs.scss:49: background-image: url(/img/png/arrow-icon-right-gray.png); NIT: Isn't this supposed to be lighter? https://codereview.adblockplus.org/29575597/diff/29575598/templates/article.tmpl File templates/article.tmpl (right): https://codereview.adblockplus.org/29575597/diff/29575598/templates/article.t... templates/article.tmpl:6: <nav class="breadcrumbs"> NIT/Suggest: Add aria-label Source: https://www.w3.org/TR/wai-aria-practices/examples/breadcrumb/index.html https://codereview.adblockplus.org/29575597/diff/29575598/templates/article.t... templates/article.tmpl:7: <ol class="container" itemscope itemtype="http://schema.org/BreadcrumbList"> NIT: I'm guessing that you copied this from an example somewhere. If so, please share your example. https://codereview.adblockplus.org/29575597/diff/29575598/templates/article.t... templates/article.tmpl:10: <img class="heading-icon" src="/img/png/logo-{{ product_id }}.png" srcset="/img/svg/logo-{{ product_id }}.svg 2x" alt="{{ product.full_name+" Logo" | translate(product_id+"-logo-alt", "Image alt text") }}"> Isn't this logo supposed to be larger? https://codereview.adblockplus.org/29575597/diff/29575598/templates/article.t... templates/article.tmpl:15: <li itemprop="itemListElement" itemscope itemtype="http://schema.org/ListItem"> NIT/Suggest: add aria-current Source: https://www.w3.org/TR/wai-aria-practices/examples/breadcrumb/index.html
> Stay awesome. :D https://codereview.adblockplus.org/29575597/diff/29575598/globals/products.py File globals/products.py (right): https://codereview.adblockplus.org/29575597/diff/29575598/globals/products.py... globals/products.py:20: "slug": "adblockplus", On 2017/10/13 11:39:54, juliandoucette wrote: > On 2017/10/13 10:30:00, ire wrote: > > Adding this was the only way I could think of to access the product slug from > > the article page. Is there another way to get the parent page? > > Hmm... The product key and slug could be the same. But I'm ok with the way that > you have implemented this. True. I'll leave it the way it is for now https://codereview.adblockplus.org/29575597/diff/29575598/static/scss/compone... File static/scss/components/_breadcrumbs.scss (right): https://codereview.adblockplus.org/29575597/diff/29575598/static/scss/compone... static/scss/components/_breadcrumbs.scss:19: padding-top: $md; On 2017/10/13 11:39:55, juliandoucette wrote: > NIT/Suggest: 1em? Done. https://codereview.adblockplus.org/29575597/diff/29575598/static/scss/compone... static/scss/components/_breadcrumbs.scss:27: display: inline-block; On 2017/10/13 11:39:54, juliandoucette wrote: > NIT: This will cause a whitespace between list items that will scale with the > font-size. I would use the floating box model instead. Done. https://codereview.adblockplus.org/29575597/diff/29575598/static/scss/compone... static/scss/components/_breadcrumbs.scss:30: color: $gray-dark; On 2017/10/13 11:39:55, juliandoucette wrote: > NIT: Isn't the blue link supposed to be grey too? What blue link? https://codereview.adblockplus.org/29575597/diff/29575598/static/scss/compone... static/scss/components/_breadcrumbs.scss:32: font-weight: $bold-weight; On 2017/10/13 11:39:54, juliandoucette wrote: > This is semibold in the mockups & styleguide We don't have a semi-bold weight for the font, and I didn't think it was necessary to load a another weight only for this purpose. Do you think we should? https://codereview.adblockplus.org/29575597/diff/29575598/static/scss/compone... static/scss/components/_breadcrumbs.scss:38: padding-right: 0; On 2017/10/13 11:39:55, juliandoucette wrote: > SuperNIT: shorthand padding causes less characters (we should decide which way > to go in our coding style if we haven't already - not that it matters very much) I agree we should add it to our coding style. In this case, I prefer this way because it's clearer that I'm overriding the padding-right that's there by default. Since in the ltr state, I'm only applying padding to one side, I want that to be the same here. Instead of doing padding-right on ltr, then specifying padding on all sides for rtl. > PS: "SuperNIT" (which I made up?) means less than a NIT not more than a NIT Ack. https://codereview.adblockplus.org/29575597/diff/29575598/static/scss/compone... static/scss/components/_breadcrumbs.scss:39: padding-left: $lg; On 2017/10/13 11:39:54, juliandoucette wrote: > NIT/Suggest: 2em? Done. https://codereview.adblockplus.org/29575597/diff/29575598/static/scss/compone... static/scss/components/_breadcrumbs.scss:45: top: 0.25em; On 2017/10/13 11:39:55, juliandoucette wrote: > NIT: This looks slightly too low > Suggest: explain this value via comment Done. https://codereview.adblockplus.org/29575597/diff/29575598/static/scss/compone... static/scss/components/_breadcrumbs.scss:46: right: $lg / 4; On 2017/10/13 11:39:55, juliandoucette wrote: > NIT/Suggest: Explain this via comment. Done. https://codereview.adblockplus.org/29575597/diff/29575598/static/scss/compone... static/scss/components/_breadcrumbs.scss:49: background-image: url(/img/png/arrow-icon-right-gray.png); On 2017/10/13 11:39:54, juliandoucette wrote: > NIT: Isn't this supposed to be lighter? Done. https://codereview.adblockplus.org/29575597/diff/29575598/templates/article.tmpl File templates/article.tmpl (right): https://codereview.adblockplus.org/29575597/diff/29575598/templates/article.t... templates/article.tmpl:6: <nav class="breadcrumbs"> On 2017/10/13 11:39:55, juliandoucette wrote: > NIT/Suggest: Add aria-label > > Source: https://www.w3.org/TR/wai-aria-practices/examples/breadcrumb/index.html Done. https://codereview.adblockplus.org/29575597/diff/29575598/templates/article.t... templates/article.tmpl:7: <ol class="container" itemscope itemtype="http://schema.org/BreadcrumbList"> On 2017/10/13 11:39:55, juliandoucette wrote: > NIT: I'm guessing that you copied this from an example somewhere. If so, please > share your example. It was from the link you shared in issue #5440 actually. Here's the link to the example https://search.google.com/structured-data/testing-tool https://codereview.adblockplus.org/29575597/diff/29575598/templates/article.t... templates/article.tmpl:10: <img class="heading-icon" src="/img/png/logo-{{ product_id }}.png" srcset="/img/svg/logo-{{ product_id }}.svg 2x" alt="{{ product.full_name+" Logo" | translate(product_id+"-logo-alt", "Image alt text") }}"> On 2017/10/13 11:39:55, juliandoucette wrote: > Isn't this logo supposed to be larger? Done. https://codereview.adblockplus.org/29575597/diff/29575598/templates/article.t... templates/article.tmpl:15: <li itemprop="itemListElement" itemscope itemtype="http://schema.org/ListItem"> On 2017/10/13 11:39:55, juliandoucette wrote: > NIT/Suggest: add aria-current > > Source: https://www.w3.org/TR/wai-aria-practices/examples/breadcrumb/index.html Done. https://codereview.adblockplus.org/29575597/diff/29578668/templates/article.tmpl File templates/article.tmpl (right): https://codereview.adblockplus.org/29575597/diff/29578668/templates/article.t... templates/article.tmpl:10: <img class="heading-icon" src="/img/png/logo-{{ product_id }}.png" srcset="/img/svg/logo-{{ product_id }}.svg 2x" alt="{{ product.full_name+" Logo" | translate(product_id+"-logo-alt", "Image alt text") }}"> This icon is now misaligned. I already created an issue to handle this before, https://issues.adblockplus.org/ticket/5848 , so I'll fix this then
https://codereview.adblockplus.org/29575597/diff/29575598/static/scss/compone... File static/scss/components/_breadcrumbs.scss (right): https://codereview.adblockplus.org/29575597/diff/29575598/static/scss/compone... static/scss/components/_breadcrumbs.scss:30: color: $gray-dark; On 2017/10/16 14:17:41, ire wrote: > On 2017/10/13 11:39:55, juliandoucette wrote: > > NIT: Isn't the blue link supposed to be grey too? > > What blue link? The first breadcrumb is blue and the second is dark grey. (I probably could have placed this comment somewhere else) https://codereview.adblockplus.org/29575597/diff/29575598/static/scss/compone... static/scss/components/_breadcrumbs.scss:32: font-weight: $bold-weight; On 2017/10/16 14:17:41, ire wrote: > On 2017/10/13 11:39:54, juliandoucette wrote: > > This is semibold in the mockups & styleguide > > We don't have a semi-bold weight for the font, and I didn't think it was > necessary to load a another weight only for this purpose. Do you think we > should? I think that all weights are supposed to be semi-bold according to the styleguide? (And I think this will look better if semi-bold is distinguishable from regular.)
https://codereview.adblockplus.org/29575597/diff/29575598/static/scss/compone... File static/scss/components/_breadcrumbs.scss (right): https://codereview.adblockplus.org/29575597/diff/29575598/static/scss/compone... static/scss/components/_breadcrumbs.scss:30: color: $gray-dark; On 2017/10/17 11:15:26, juliandoucette wrote: > On 2017/10/16 14:17:41, ire wrote: > > On 2017/10/13 11:39:55, juliandoucette wrote: > > > NIT: Isn't the blue link supposed to be grey too? > > > > What blue link? > > The first breadcrumb is blue and the second is dark grey. > > (I probably could have placed this comment somewhere else) Oh it does't show that way for me. I'll move this to styling the link instead of the li https://codereview.adblockplus.org/29575597/diff/29575598/static/scss/compone... static/scss/components/_breadcrumbs.scss:30: color: $gray-dark; On 2017/10/17 11:15:26, juliandoucette wrote: > On 2017/10/16 14:17:41, ire wrote: > > On 2017/10/13 11:39:55, juliandoucette wrote: > > > NIT: Isn't the blue link supposed to be grey too? > > > > What blue link? > > The first breadcrumb is blue and the second is dark grey. > > (I probably could have placed this comment somewhere else) Done. https://codereview.adblockplus.org/29575597/diff/29575598/static/scss/compone... static/scss/components/_breadcrumbs.scss:32: font-weight: $bold-weight; On 2017/10/17 11:15:26, juliandoucette wrote: > On 2017/10/16 14:17:41, ire wrote: > > On 2017/10/13 11:39:54, juliandoucette wrote: > > > This is semibold in the mockups & styleguide > > > > We don't have a semi-bold weight for the font, and I didn't think it was > > necessary to load a another weight only for this purpose. Do you think we > > should? > > I think that all weights are supposed to be semi-bold according to the > styleguide? > > (And I think this will look better if semi-bold is distinguishable from > regular.) As we discussed will handle in this issue https://issues.adblockplus.org/ticket/5903
(Can you please rebase this?)
Only one not-NIT. See the first comment / the only comment in _breadcrumbs.scss. https://codereview.adblockplus.org/29575597/diff/29586673/static/scss/compone... File static/scss/components/_breadcrumb.scss (right): https://codereview.adblockplus.org/29575597/diff/29586673/static/scss/compone... static/scss/components/_breadcrumb.scss:25: .breadcrumb li I think that this text is too close together, vertically, when broken? https://codereview.adblockplus.org/29575597/diff/29586673/templates/article.tmpl File templates/article.tmpl (right): https://codereview.adblockplus.org/29575597/diff/29586673/templates/article.t... templates/article.tmpl:6: <nav aria-label="{{ "Breadcrumb" | translate("breadcrumb-label", "Label") }}" class="breadcrumb"> NIT: "Breadcrumb" is singular but you are referring to a list of 0 to many breadcrumbs. Suggest: Something more friendly e.g. "You are here" Acknowledging that you probably got this idea from the w3 ARIA example and that the logic on that page (labeling the type of navigation) makes sense. Please consider my suggestion and do whatever you think is right. I think that both of these options are good. https://codereview.adblockplus.org/29575597/diff/29586673/templates/article.t... templates/article.tmpl:9: <a itemscope itemtype="http://schema.org/Thing" itemprop="item" href="{{ product.slug }}"> Suggest: http://schema.org/Product https://codereview.adblockplus.org/29575597/diff/29586673/templates/article.t... templates/article.tmpl:10: <img class="heading-icon" src="/img/png/logo-{{ product_id }}.png" srcset="/img/svg/logo-{{ product_id }}.svg 2x" alt="{{ product.full_name+" Logo" | translate(product_id+"-logo-alt", "Image alt text") }}"> NIT: I don't know why extra spaces with text-decoration bother me so much. I want to suggest that you place this image via CSS [to avoid this issue, because the label alone is probably enough]. But I acknowledge that I'm probably being too picky. You could address this separately and/or not at all.
New patch uploaded. The .heading-icon element is messing things up more, so I will handle that issue asap. https://codereview.adblockplus.org/29575597/diff/29586673/static/scss/compone... File static/scss/components/_breadcrumb.scss (right): https://codereview.adblockplus.org/29575597/diff/29586673/static/scss/compone... static/scss/components/_breadcrumb.scss:25: .breadcrumb li On 2017/10/24 09:51:13, juliandoucette wrote: > I think that this text is too close together, vertically, when broken? Done. https://codereview.adblockplus.org/29575597/diff/29586673/templates/article.tmpl File templates/article.tmpl (right): https://codereview.adblockplus.org/29575597/diff/29586673/templates/article.t... templates/article.tmpl:6: <nav aria-label="{{ "Breadcrumb" | translate("breadcrumb-label", "Label") }}" class="breadcrumb"> On 2017/10/24 09:51:13, juliandoucette wrote: > NIT: "Breadcrumb" is singular but you are referring to a list of 0 to many > breadcrumbs. > > Suggest: Something more friendly e.g. "You are here" > > Acknowledging that you probably got this idea from the w3 ARIA example and that > the logic on that page (labeling the type of navigation) makes sense. Please > consider my suggestion and do whatever you think is right. I think that both of > these options are good. Yes I actually had it as "Breadcrumbs" before, but I changed it because of the example. I would prefer to follow the example in this case. https://codereview.adblockplus.org/29575597/diff/29586673/templates/article.t... templates/article.tmpl:9: <a itemscope itemtype="http://schema.org/Thing" itemprop="item" href="{{ product.slug }}"> On 2017/10/24 09:51:14, juliandoucette wrote: > Suggest: http://schema.org/Product The ABP Help Center Home isn't really a product. I think that scheme would be more suitable for linking to abp.org, not help.eyeo.com/abp https://codereview.adblockplus.org/29575597/diff/29586673/templates/article.t... templates/article.tmpl:10: <img class="heading-icon" src="/img/png/logo-{{ product_id }}.png" srcset="/img/svg/logo-{{ product_id }}.svg 2x" alt="{{ product.full_name+" Logo" | translate(product_id+"-logo-alt", "Image alt text") }}"> On 2017/10/24 09:51:14, juliandoucette wrote: > NIT: I don't know why extra spaces with text-decoration bother me so much. I > want to suggest that you place this image via CSS [to avoid this issue, because > the label alone is probably enough]. But I acknowledge that I'm probably being > too picky. You could address this separately and/or not at all. I'll address this in this issue with the alignment of these icons https://issues.adblockplus.org/ticket/5848
LGTM https://codereview.adblockplus.org/29575597/diff/29586673/templates/article.tmpl File templates/article.tmpl (right): https://codereview.adblockplus.org/29575597/diff/29586673/templates/article.t... templates/article.tmpl:6: <nav aria-label="{{ "Breadcrumb" | translate("breadcrumb-label", "Label") }}" class="breadcrumb"> On 2017/10/25 10:00:06, ire wrote: > On 2017/10/24 09:51:13, juliandoucette wrote: > > NIT: "Breadcrumb" is singular but you are referring to a list of 0 to many > > breadcrumbs. > > > > Suggest: Something more friendly e.g. "You are here" > > > > Acknowledging that you probably got this idea from the w3 ARIA example and > that > > the logic on that page (labeling the type of navigation) makes sense. Please > > consider my suggestion and do whatever you think is right. I think that both > of > > these options are good. > > Yes I actually had it as "Breadcrumbs" before, but I changed it because of the > example. > > I would prefer to follow the example in this case. Acknowledged. https://codereview.adblockplus.org/29575597/diff/29586673/templates/article.t... templates/article.tmpl:9: <a itemscope itemtype="http://schema.org/Thing" itemprop="item" href="{{ product.slug }}"> On 2017/10/25 10:00:06, ire wrote: > On 2017/10/24 09:51:14, juliandoucette wrote: > > Suggest: http://schema.org/Product > > The ABP Help Center Home isn't really a product. I think that scheme would be > more suitable for linking to http://abp.org, not help.eyeo.com/abp Acknowledged. https://codereview.adblockplus.org/29575597/diff/29586673/templates/article.t... templates/article.tmpl:10: <img class="heading-icon" src="/img/png/logo-{{ product_id }}.png" srcset="/img/svg/logo-{{ product_id }}.svg 2x" alt="{{ product.full_name+" Logo" | translate(product_id+"-logo-alt", "Image alt text") }}"> On 2017/10/25 10:00:06, ire wrote: > On 2017/10/24 09:51:14, juliandoucette wrote: > > NIT: I don't know why extra spaces with text-decoration bother me so much. I > > want to suggest that you place this image via CSS [to avoid this issue, > because > > the label alone is probably enough]. But I acknowledge that I'm probably being > > too picky. You could address this separately and/or not at all. > > I'll address this in this issue with the alignment of these icons > https://issues.adblockplus.org/ticket/5848 Acknowledged. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
