Issue 29401619: Issue 4963 - Wrong size used for text logo on acceptableads.com (Closed)
|
Created:
April 3, 2017, 9:39 p.m. by juliandoucette Modified:
June 22, 2017, 1:48 p.m. Reviewers:
saroyanm CC:
Thomas Greiner, ire Base URL:
https://hg.adblockplus.org/web.acceptableads.com Visibility:
Public. |
DescriptionIssue 4963 - Wrong size used for text logo on acceptableads.com
Patch Set 1 #
Total comments: 9
Patch Set 2 : Addressed comments #
Total comments: 12
Patch Set 3 : Addressed comments #Patch Set 4 : Changed non-space to word-spacing #
Total comments: 1
MessagesTotal messages: 14
On 2017/04/03 21:39:22, juliandoucette wrote: Small change, but it's worth considering. I'm not 100% sure about using h1 or cross browser support here.
https://codereview.adblockplus.org/29401619/diff/29401620/includes/navbar.tmpl File includes/navbar.tmpl (left): https://codereview.adblockplus.org/29401619/diff/29401620/includes/navbar.tmp... includes/navbar.tmpl:22: <img src="/img/png/logo-black.png" srcset="/img/png/logo-black.png, /img/svg/logo-black.svg 2x" aria-hidden="true"> We still have this image in sidebar to be shown on mobile. https://codereview.adblockplus.org/29401619/diff/29401620/static/scss/layout/... File static/scss/layout/_navbar.scss (right): https://codereview.adblockplus.org/29401619/diff/29401620/static/scss/layout/... static/scss/layout/_navbar.scss:43: font-size: 17px; Detail: in ticket it's mentioned 16px, I assume you adjusted, I don't mind the change, but I think make sense to update in the ticket as well. https://codereview.adblockplus.org/29401619/diff/29401620/static/scss/layout/... static/scss/layout/_navbar.scss:57: margin-left: -2px; Is there a reason why you are not removing the space instead ?
(I'm going to skip this one today because it's not trivial. I'll get to it early next week.)
Updated (finally). I tried to keep this change as small as possible because of it's priority. I would love to re-implement #sidebar and #navbar... Just not in this patchset. https://codereview.adblockplus.org/29401619/diff/29401620/includes/navbar.tmpl File includes/navbar.tmpl (left): https://codereview.adblockplus.org/29401619/diff/29401620/includes/navbar.tmp... includes/navbar.tmpl:22: <img src="/img/png/logo-black.png" srcset="/img/png/logo-black.png, /img/svg/logo-black.svg 2x" aria-hidden="true"> On 2017/04/28 10:19:48, saroyanm wrote: > We still have this image in sidebar to be shown on mobile. Acknowledged. I would have to re-write a significant portion of the sidebar to avoid this duplication. As a result, I vote we address it in a separate issue. https://codereview.adblockplus.org/29401619/diff/29401620/static/scss/layout/... File static/scss/layout/_navbar.scss (right): https://codereview.adblockplus.org/29401619/diff/29401620/static/scss/layout/... static/scss/layout/_navbar.scss:43: font-size: 17px; On 2017/04/28 10:19:48, saroyanm wrote: > Detail: in ticket it's mentioned 16px, I assume you adjusted, I don't mind the > change, but I think make sense to update in the ticket as well. Done. https://codereview.adblockplus.org/29401619/diff/29401620/static/scss/layout/... static/scss/layout/_navbar.scss:57: margin-left: -2px; On 2017/04/28 10:19:48, saroyanm wrote: > Is there a reason why you are not removing the space instead ? Acknowledged. We determined that removing the space was the best option in our last meeting. However, it doesn't look good when we do. As a result, I added a ".non-space" span with 1px width. https://codereview.adblockplus.org/29401619/diff/29445670/static/scss/layout/... File static/scss/layout/_sidebar.scss (right): https://codereview.adblockplus.org/29401619/diff/29445670/static/scss/layout/... static/scss/layout/_sidebar.scss:85: #sidebar-menus I moved this from #sidebar to #sidebar-menus to avoid overriding it in #sidebar .site-title.
https://codereview.adblockplus.org/29401619/diff/29445670/includes/navbar.tmpl File includes/navbar.tmpl (right): https://codereview.adblockplus.org/29401619/diff/29445670/includes/navbar.tmp... includes/navbar.tmpl:20: <h1 class="site-title">{{ "index" | linkify(id="navbar-logo") }}Acceptable<span class="non-space"> </span><strong>Ads</strong></a></h1> What about ?: <h1 class="site-title"> <span>Acceptable</span><strong>Ads</strong> </h1> .site-title { font-size: 0px; } .site-title > * { font-size: 17px; } See discussion: https://stackoverflow.com/questions/5078239/how-to-remove-the-space-between-i... https://codereview.adblockplus.org/29401619/diff/29445670/static/scss/layout/... File static/scss/layout/_navbar.scss (right): https://codereview.adblockplus.org/29401619/diff/29445670/static/scss/layout/... static/scss/layout/_navbar.scss:67: html[dir="rtl"] #navbar-logo "#navbar-logo" Item is a duplication, let's use ".site-title" instead and remove the ID from the elements if no more in use. https://codereview.adblockplus.org/29401619/diff/29445670/static/scss/layout/... File static/scss/layout/_sidebar.scss (left): https://codereview.adblockplus.org/29401619/diff/29445670/static/scss/layout/... static/scss/layout/_sidebar.scss:202: #sidebar-title Seems like we are not referring to the item by it's ID anymore, I don't think we need to keep the element Identifiable anymore.
Thanks Manvel :) A new patchset is ready. https://codereview.adblockplus.org/29401619/diff/29445670/includes/navbar.tmpl File includes/navbar.tmpl (right): https://codereview.adblockplus.org/29401619/diff/29445670/includes/navbar.tmp... includes/navbar.tmpl:20: <h1 class="site-title">{{ "index" | linkify(id="navbar-logo") }}Acceptable<span class="non-space"> </span><strong>Ads</strong></a></h1> On 2017/06/01 13:09:54, saroyanm wrote: > What about ?: > > <h1 class="site-title"> > <span>Acceptable</span><strong>Ads</strong> > </h1> > > .site-title > { > font-size: 0px; > } > .site-title > * > { > font-size: 17px; > } > > See discussion: > https://stackoverflow.com/questions/5078239/how-to-remove-the-space-between-i... I tried that already and I didn't think it looked right. The 1px space is necessary IMO. Please look yourself and tell me if you think otherwise. https://codereview.adblockplus.org/29401619/diff/29445670/static/scss/layout/... File static/scss/layout/_navbar.scss (right): https://codereview.adblockplus.org/29401619/diff/29445670/static/scss/layout/... static/scss/layout/_navbar.scss:67: html[dir="rtl"] #navbar-logo On 2017/06/01 13:09:54, saroyanm wrote: > "#navbar-logo" Item is a duplication, let's use ".site-title" instead and remove > the ID from the elements if no more in use. Done. Good find. https://codereview.adblockplus.org/29401619/diff/29445670/static/scss/layout/... File static/scss/layout/_sidebar.scss (left): https://codereview.adblockplus.org/29401619/diff/29445670/static/scss/layout/... static/scss/layout/_sidebar.scss:202: #sidebar-title On 2017/06/01 13:09:54, saroyanm wrote: > Seems like we are not referring to the item by it's ID anymore, I don't think we > need to keep the element Identifiable anymore. Done. Good find.
I don't think we need the element in between. In case I don't understand the issue we can have a quick call and fix this quickly, it should be easy to fix and push this change. https://codereview.adblockplus.org/29401619/diff/29401620/includes/navbar.tmpl File includes/navbar.tmpl (left): https://codereview.adblockplus.org/29401619/diff/29401620/includes/navbar.tmp... includes/navbar.tmpl:22: <img src="/img/png/logo-black.png" srcset="/img/png/logo-black.png, /img/svg/logo-black.svg 2x" aria-hidden="true"> On 2017/05/22 19:18:33, juliandoucette wrote: > On 2017/04/28 10:19:48, saroyanm wrote: > > We still have this image in sidebar to be shown on mobile. > > Acknowledged. > > I would have to re-write a significant portion of the sidebar to avoid this > duplication. As a result, I vote we address it in a separate issue. Do we have an issue already ? https://codereview.adblockplus.org/29401619/diff/29445670/includes/navbar.tmpl File includes/navbar.tmpl (right): https://codereview.adblockplus.org/29401619/diff/29445670/includes/navbar.tmp... includes/navbar.tmpl:20: <h1 class="site-title">{{ "index" | linkify(id="navbar-logo") }}Acceptable<span class="non-space"> </span><strong>Ads</strong></a></h1> On 2017/06/07 15:42:52, juliandoucette wrote: > On 2017/06/01 13:09:54, saroyanm wrote: > > What about ?: > > > > <h1 class="site-title"> > > <span>Acceptable</span><strong>Ads</strong> > > </h1> > > > > .site-title > > { > > font-size: 0px; > > } > > .site-title > * > > { > > font-size: 17px; > > } > > > > See discussion: > > > https://stackoverflow.com/questions/5078239/how-to-remove-the-space-between-i... > > I tried that already and I didn't think it looked right. The 1px space is > necessary IMO. Please look yourself and tell me if you think otherwise. I thought you were fighting the space in between, What about: .site-title strong { margin: 0px 1px; } I'm not really sure if we will need additional element to achieve the result.
Thanks Manvel! https://codereview.adblockplus.org/29401619/diff/29401620/includes/navbar.tmpl File includes/navbar.tmpl (left): https://codereview.adblockplus.org/29401619/diff/29401620/includes/navbar.tmp... includes/navbar.tmpl:22: <img src="/img/png/logo-black.png" srcset="/img/png/logo-black.png, /img/svg/logo-black.svg 2x" aria-hidden="true"> > Do we have an issue already ? Not yet. I will create one before I upload the next patchset. https://codereview.adblockplus.org/29401619/diff/29445670/includes/navbar.tmpl File includes/navbar.tmpl (right): https://codereview.adblockplus.org/29401619/diff/29445670/includes/navbar.tmp... includes/navbar.tmpl:20: <h1 class="site-title">{{ "index" | linkify(id="navbar-logo") }}Acceptable<span class="non-space"> </span><strong>Ads</strong></a></h1> > I thought you were fighting the space in between, > > What about: > .site-title strong > { > margin: 0px 1px; > } > > I'm not really sure if we will need additional element to achieve the result. Fuck me... You're right :D - Thank you!
See comment below. https://codereview.adblockplus.org/29401619/diff/29445670/includes/navbar.tmpl File includes/navbar.tmpl (right): https://codereview.adblockplus.org/29401619/diff/29445670/includes/navbar.tmp... includes/navbar.tmpl:20: <h1 class="site-title">{{ "index" | linkify(id="navbar-logo") }}Acceptable<span class="non-space"> </span><strong>Ads</strong></a></h1> > I thought you were fighting the space in between, > > What about: > .site-title strong > { > margin: 0px 1px; > } > > I'm not really sure if we will need additional element to achieve the result. I have to backpedal here... Actually, I am fighting the space in between. "Acceptable Ads" is two words. I know that we tested one screen reader - but that doesn't confirm that this works as expected using others. Or that search engines will interpret this correctly.
https://codereview.adblockplus.org/29401619/diff/29445670/includes/navbar.tmpl File includes/navbar.tmpl (right): https://codereview.adblockplus.org/29401619/diff/29445670/includes/navbar.tmp... includes/navbar.tmpl:20: <h1 class="site-title">{{ "index" | linkify(id="navbar-logo") }}Acceptable<span class="non-space"> </span><strong>Ads</strong></a></h1> On 2017/06/13 11:59:30, juliandoucette wrote: > > I thought you were fighting the space in between, > > > > What about: > > .site-title strong > > { > > margin: 0px 1px; > > } > > > > I'm not really sure if we will need additional element to achieve the result. > > I have to backpedal here... > > Actually, I am fighting the space in between. "Acceptable Ads" is two words. I > know that we tested one screen reader - but that doesn't confirm that this works > as expected using others. Or that search engines will interpret this correctly. Acknowledged. What about using "word-spacing" property instead ?
Thanks Manvel :) https://codereview.adblockplus.org/29401619/diff/29445670/includes/navbar.tmpl File includes/navbar.tmpl (right): https://codereview.adblockplus.org/29401619/diff/29445670/includes/navbar.tmp... includes/navbar.tmpl:20: <h1 class="site-title">{{ "index" | linkify(id="navbar-logo") }}Acceptable<span class="non-space"> </span><strong>Ads</strong></a></h1> > Acknowledged. > > What about using "word-spacing" property instead ? Done. Good idea :)
https://codereview.adblockplus.org/29401619/diff/29401620/includes/navbar.tmpl File includes/navbar.tmpl (left): https://codereview.adblockplus.org/29401619/diff/29401620/includes/navbar.tmp... includes/navbar.tmpl:22: <img src="/img/png/logo-black.png" srcset="/img/png/logo-black.png, /img/svg/logo-black.svg 2x" aria-hidden="true"> On 2017/06/13 11:17:09, juliandoucette wrote: > > Do we have an issue already ? > > Not yet. I will create one before I upload the next patchset. Done. https://issues.adblockplus.org/ticket/5319
LGTM - with 1 small comment. https://codereview.adblockplus.org/29401619/diff/29465592/static/css/main.css File static/css/main.css (right): https://codereview.adblockplus.org/29401619/diff/29465592/static/css/main.css... static/css/main.css:1236: word-spacing: 1px; } I assume you forgot to generate latest version, as I can see it's -2px. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
