Issue 29326238: Issue 3031 - Add Adblock Browser section to First Run Page (Closed)
|
Created:
Sept. 10, 2015, 8:56 a.m. by saroyanm Modified:
Sept. 16, 2015, 2:47 p.m. CC:
Felix Dahlke Visibility:
Public. |
DescriptionIssue 3031 - Add Adblock Browser section to First Run Page
Patch Set 1 #
Total comments: 3
Patch Set 2 : Removed block for French locale #
Total comments: 24
Patch Set 3 : Changed the translatable message string IDs #Patch Set 4 : Addressed Thomas comments #
Total comments: 8
Patch Set 5 : Make section consistent, horisontal separation and small fixes #Patch Set 6 : Small fixes #
Total comments: 8
Patch Set 7 : top border fix for RTL #
Total comments: 2
Patch Set 8 : Addressed Thomas comments #
Total comments: 6
Patch Set 9 : Use border-start instead of rtl hack #
Total comments: 2
Patch Set 10 : Reverted back to table-cell #
MessagesTotal messages: 30
@Thomas can you please have a look ? https://codereview.adblockplus.org/29326238/diff/29326239/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29326239/skin/firstRun.css#n... skin/firstRun.css:191: background: linear-gradient(to top, #294e76, #6b92be); I'm not sure what is the best way to have inner border ? I think we can achieve a similar result with nested block element, but not sure if it's a nice solution. https://codereview.adblockplus.org/29326238/diff/29326239/skin/firstRun.css#n... skin/firstRun.css:213: font-size: 22px; Please note I did it according to the style guide, but I do think we at least need to be sure that in English version the text in the button is not wrapped. https://codereview.adblockplus.org/29326238/diff/29326239/skin/firstRun.css#n... skin/firstRun.css:224: #abb-promotion-block a:hover In the style guide it wasn't obvious how the :hover should be implemented, I just added some box-shadows, to make the link feel like a button.
Please note that this change shouldn't be visible if |#content:lang(fr)|.
On 2015/09/10 09:16:17, Sebastian Noack wrote: > Please note that this change shouldn't be visible if |#content:lang(fr)|. Thanks for note, done.
Changed the translatable message string IDs
https://codereview.adblockplus.org/29326238/diff/29326244/firstRun.html File firstRun.html (right): https://codereview.adblockplus.org/29326238/diff/29326244/firstRun.html#newco... firstRun.html:43: <div id="acceptable-ads-block"> Detail: There's no need to change the ID of this element. https://codereview.adblockplus.org/29326238/diff/29326244/firstRun.html#newco... firstRun.html:45: <p id="acceptableAdsExplanation" class="i18n_firstRun_acceptableAdsExplanation"></p> Detail: Inconsistent naming of element IDs. https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css#n... skin/firstRun.css:152: } Why do you need to deviate so much from the default style for sections? For instance, the general section should still have the same maximum width and paddings as any other section. https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css#n... skin/firstRun.css:195: background: -prefix-gradient(to top, #294e76, #6b92be); I suppose that "-prefix-" is a placeholder for vendor prefixes. https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css#n... skin/firstRun.css:213: width: 1px; What is this supposed to do? Looks like it's a workaround for something. https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css#n... skin/firstRun.css:223: #abb-promotion-block a div > div:not(:first-child) Detail: That's a situation where assigning a class name to an element is more descriptive. Basically, what you want to target is a ".subtitle" here and ".title" above, for example. https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css#n... skin/firstRun.css:232: -webkit-box-shadow: 0px 0px 5px #5D5D5D; According to MDN the prefixes have been removed in Firefox 4 and Chrome 10 so not necessary to include. https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css#n... skin/firstRun.css:250: } Basically, all this block does is override `display: table` so the rest appears to be unnecessary. Similar situation with `#general > div` below.
Thomas can you please have a look on the comments I left in: https://codereview.adblockplus.org/29326238/diff/29326239/skin/firstRun.css Just would like to be sure that you have noticed them. https://codereview.adblockplus.org/29326238/diff/29326244/firstRun.html File firstRun.html (right): https://codereview.adblockplus.org/29326238/diff/29326244/firstRun.html#newco... firstRun.html:43: <div id="acceptable-ads-block"> On 2015/09/11 18:11:59, Thomas Greiner wrote: > Detail: There's no need to change the ID of this element. I tried to make the consistent, in the share section we have "block" postfix on the according element IDs. https://codereview.adblockplus.org/29326238/diff/29326244/firstRun.html#newco... firstRun.html:45: <p id="acceptableAdsExplanation" class="i18n_firstRun_acceptableAdsExplanation"></p> On 2015/09/11 18:11:59, Thomas Greiner wrote: > Detail: Inconsistent naming of element IDs. I guess you mean the dashes separation ? Done. Please note that this ID was here beforehand, but good that we can fix that inconsistency here. https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css#n... skin/firstRun.css:152: } On 2015/09/11 18:12:00, Thomas Greiner wrote: > Why do you need to deviate so much from the default style for sections? For > instance, the general section should still have the same maximum width and > paddings as any other section. I did this because I wanted to setup equal right and left side padding for the separated contents (AA and ABB promo.), but in general I think we need to assign box-content: border-box; to section blocks and change their width to 960px, but to be more clear I think we need to assign that changes to all elements on the page, but we will need to align the elements afterward, if you think that make sense to do it in current review I'll apply box-content to each element in the page and fix sizes. https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css#n... skin/firstRun.css:195: background: -prefix-gradient(to top, #294e76, #6b92be); On 2015/09/11 18:11:59, Thomas Greiner wrote: > I suppose that "-prefix-" is a placeholder for vendor prefixes. That's stupid sorry :D Done. https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css#n... skin/firstRun.css:213: width: 1px; On 2015/09/11 18:11:59, Thomas Greiner wrote: > What is this supposed to do? Looks like it's a workaround for something. Yes, I need the first cell to be adjusted to the width of it's content, you can see the effect on small screens when the second cell have room to expand. https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css#n... skin/firstRun.css:223: #abb-promotion-block a div > div:not(:first-child) On 2015/09/11 18:12:00, Thomas Greiner wrote: > Detail: That's a situation where assigning a class name to an element is more > descriptive. Basically, what you want to target is a ".subtitle" here and > ".title" above, for example. Done. https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css#n... skin/firstRun.css:232: -webkit-box-shadow: 0px 0px 5px #5D5D5D; On 2015/09/11 18:11:59, Thomas Greiner wrote: > According to MDN the prefixes have been removed in Firefox 4 and Chrome 10 so > not necessary to include. Safari 5.0 ? The prefixes looks to be removed from Safari 5.1. https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css#n... skin/firstRun.css:250: } On 2015/09/11 18:11:59, Thomas Greiner wrote: > Basically, all this block does is override `display: table` so the rest appears > to be unnecessary. > > Similar situation with `#general > div` below. Ahh right, hmm. Done.
https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css#n... skin/firstRun.css:152: } On 2015/09/15 09:42:37, saroyanm wrote: > On 2015/09/11 18:12:00, Thomas Greiner wrote: > > Why do you need to deviate so much from the default style for sections? For > > instance, the general section should still have the same maximum width and > > paddings as any other section. > > I did this because I wanted to setup equal right and left side padding for the > separated contents (AA and ABB promo.), but in general I think we need to assign > box-content: border-box; to section blocks and change their width to 960px, but > to be more clear I think we need to assign that changes to all elements on the > page, but we will need to align the elements afterward, if you think that make > sense to do it in current review I'll apply box-content to each element in the > page and fix sizes. Not to each element, no. It should only be applied where necessary. Generally, it's not straight-forward to see sections having a maximum width of `760px` while another one has `960px` but still look like they have the same width on the screen. https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css#n... skin/firstRun.css:213: width: 1px; On 2015/09/15 09:42:37, saroyanm wrote: > On 2015/09/11 18:11:59, Thomas Greiner wrote: > > What is this supposed to do? Looks like it's a workaround for something. > > Yes, I need the first cell to be adjusted to the width of it's content, you can > see the effect on small screens when the second cell have room to expand. The behavior on small screens hasn't been specified so I consulted with Sven. The button should not take up the entire width but instead behave the same as on larger screens and be centered. Making those changes will eliminate the need for this workaround. https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css#n... skin/firstRun.css:232: -webkit-box-shadow: 0px 0px 5px #5D5D5D; On 2015/09/15 09:42:37, saroyanm wrote: > On 2015/09/11 18:11:59, Thomas Greiner wrote: > > According to MDN the prefixes have been removed in Firefox 4 and Chrome 10 so > > not necessary to include. > > Safari 5.0 ? The prefixes looks to be removed from Safari 5.1. 1) `-moz-xxx` is still unnecessary. 2) AFAIK we don't support Safari 5.0. https://codereview.adblockplus.org/29326238/diff/29327628/locale/en-US/firstR... File locale/en-US/firstRun.json (right): https://codereview.adblockplus.org/29326238/diff/29327628/locale/en-US/firstR... locale/en-US/firstRun.json:12: "message": "Get Adblock Browser here" Sorry, I didn't notice that one before but "Browser" should be wrapped inside a `<strong>` tag. However, since this is not crucial, we're under a bit of time pressure and have already worked on the translations for those, feel free to ignore this. I still wanted to mention it for completeness' sake. https://codereview.adblockplus.org/29326238/diff/29327628/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29327628/skin/firstRun.css#n... skin/firstRun.css:195: background: -webkit-gradient(to top, #294e76, #6b92be); Two more issues: 1) According to MDN Safari 5.1 requires `-webkit-xxx` but without support for the `to xxx` syntax. 2) It should be `-webkit-linear-gradient` and not `-webkit-gradient`. https://codereview.adblockplus.org/29326238/diff/29327628/skin/firstRun.css#n... skin/firstRun.css:218: font-size: 22px; This font size causes the text to be split up into two lines. I checked back with Sven and reducing it to `21px` would be a quick workaround to at least ensure that the english version of the text stays in one line.
https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css#n... skin/firstRun.css:152: } On 2015/09/15 11:19:31, Thomas Greiner wrote: > On 2015/09/15 09:42:37, saroyanm wrote: > > On 2015/09/11 18:12:00, Thomas Greiner wrote: > > > Why do you need to deviate so much from the default style for sections? For > > > instance, the general section should still have the same maximum width and > > > paddings as any other section. > > > > I did this because I wanted to setup equal right and left side padding for the > > separated contents (AA and ABB promo.), but in general I think we need to > assign > > box-content: border-box; to section blocks and change their width to 960px, > but > > to be more clear I think we need to assign that changes to all elements on the > > page, but we will need to align the elements afterward, if you think that make > > sense to do it in current review I'll apply box-content to each element in the > > page and fix sizes. > > Not to each element, no. It should only be applied where necessary. Generally, > it's not straight-forward to see sections having a maximum width of `760px` > while another one has `960px` but still look like they have the same width on > the screen. I've made a fix accordingly so we will not need to apply width separately. https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css#n... skin/firstRun.css:213: width: 1px; On 2015/09/15 11:19:31, Thomas Greiner wrote: > On 2015/09/15 09:42:37, saroyanm wrote: > > On 2015/09/11 18:11:59, Thomas Greiner wrote: > > > What is this supposed to do? Looks like it's a workaround for something. > > > > Yes, I need the first cell to be adjusted to the width of it's content, you > can > > see the effect on small screens when the second cell have room to expand. > > The behavior on small screens hasn't been specified so I consulted with Sven. > The button should not take up the entire width but instead behave the same as on > larger screens and be centered. > > Making those changes will eliminate the need for this workaround. Well in that case we will need to specify width I guess, should I use 340px ? https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css#n... skin/firstRun.css:232: -webkit-box-shadow: 0px 0px 5px #5D5D5D; On 2015/09/15 11:19:31, Thomas Greiner wrote: > On 2015/09/15 09:42:37, saroyanm wrote: > > On 2015/09/11 18:11:59, Thomas Greiner wrote: > > > According to MDN the prefixes have been removed in Firefox 4 and Chrome 10 > so > > > not necessary to include. > > > > Safari 5.0 ? The prefixes looks to be removed from Safari 5.1. > > 1) `-moz-xxx` is still unnecessary. > 2) AFAIK we don't support Safari 5.0. Done. https://codereview.adblockplus.org/29326238/diff/29327628/locale/en-US/firstR... File locale/en-US/firstRun.json (right): https://codereview.adblockplus.org/29326238/diff/29327628/locale/en-US/firstR... locale/en-US/firstRun.json:12: "message": "Get Adblock Browser here" On 2015/09/15 11:19:31, Thomas Greiner wrote: > Sorry, I didn't notice that one before but "Browser" should be wrapped inside a > `<strong>` tag. However, since this is not crucial, we're under a bit of time > pressure and have already worked on the translations for those, feel free to > ignore this. > > I still wanted to mention it for completeness' sake. Yes I think we can live without having it here while we have that time pressure, so leaving as it is for now. https://codereview.adblockplus.org/29326238/diff/29327628/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29327628/skin/firstRun.css#n... skin/firstRun.css:195: background: -webkit-gradient(to top, #294e76, #6b92be); On 2015/09/15 11:19:32, Thomas Greiner wrote: > Two more issues: > > 1) According to MDN Safari 5.1 requires `-webkit-xxx` but without support for > the `to xxx` syntax. > 2) It should be `-webkit-linear-gradient` and not `-webkit-gradient`. Done. https://codereview.adblockplus.org/29326238/diff/29327628/skin/firstRun.css#n... skin/firstRun.css:218: font-size: 22px; On 2015/09/15 11:19:31, Thomas Greiner wrote: > This font size causes the text to be split up into two lines. I checked back > with Sven and reducing it to `21px` would be a quick workaround to at least > ensure that the english version of the text stays in one line. Done.
https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css#n... skin/firstRun.css:213: width: 1px; On 2015/09/15 12:43:31, saroyanm wrote: > On 2015/09/15 11:19:31, Thomas Greiner wrote: > > On 2015/09/15 09:42:37, saroyanm wrote: > > > On 2015/09/11 18:11:59, Thomas Greiner wrote: > > > > What is this supposed to do? Looks like it's a workaround for something. > > > > > > Yes, I need the first cell to be adjusted to the width of it's content, you > > can > > > see the effect on small screens when the second cell have room to expand. > > > > The behavior on small screens hasn't been specified so I consulted with Sven. > > The button should not take up the entire width but instead behave the same as > on > > larger screens and be centered. > > > > Making those changes will eliminate the need for this workaround. > > Well in that case we will need to specify width I guess, should I use 340px ? No, you shouldn't need to specify a width. If you're encountering issues, you could still change `#abb-promotion-block a` from `display: table` to `display: inline-block`. https://codereview.adblockplus.org/29326238/diff/29327628/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29327628/skin/firstRun.css#n... skin/firstRun.css:195: background: -webkit-gradient(to top, #294e76, #6b92be); On 2015/09/15 12:43:31, saroyanm wrote: > On 2015/09/15 11:19:32, Thomas Greiner wrote: > > Two more issues: > > > > 1) According to MDN Safari 5.1 requires `-webkit-xxx` but without support for > > the `to xxx` syntax. > > 2) It should be `-webkit-linear-gradient` and not `-webkit-gradient`. > > Done. Nope, while the `to X` syntax defines to which point a gradient should flow, the old syntax defines where it originates. Therefore you need to either change "top" to "bottom" or reverse the order of the color values. See also https://developer.mozilla.org/en-US/docs/Web/CSS/linear-gradient#History_of_t... "Unfortunately, the addition of the <angle> values to the syntax introduced an incoherence: the angle indicates a destination, but the keywords indicate a starting point.# This was fixed by a new syntax where the keyword are directions too, and preceded by the to keyword."
https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29326244/skin/firstRun.css#n... skin/firstRun.css:213: width: 1px; On 2015/09/15 14:15:50, Thomas Greiner wrote: > On 2015/09/15 12:43:31, saroyanm wrote: > > On 2015/09/15 11:19:31, Thomas Greiner wrote: > > > On 2015/09/15 09:42:37, saroyanm wrote: > > > > On 2015/09/11 18:11:59, Thomas Greiner wrote: > > > > > What is this supposed to do? Looks like it's a workaround for something. > > > > > > > > Yes, I need the first cell to be adjusted to the width of it's content, > you > > > can > > > > see the effect on small screens when the second cell have room to expand. > > > > > > The behavior on small screens hasn't been specified so I consulted with > Sven. > > > The button should not take up the entire width but instead behave the same > as > > on > > > larger screens and be centered. > > > > > > Making those changes will eliminate the need for this workaround. > > > > Well in that case we will need to specify width I guess, should I use 340px ? > > No, you shouldn't need to specify a width. If you're encountering issues, you > could still change `#abb-promotion-block a` from `display: table` to `display: > inline-block`. Done. https://codereview.adblockplus.org/29326238/diff/29327628/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29327628/skin/firstRun.css#n... skin/firstRun.css:195: background: -webkit-gradient(to top, #294e76, #6b92be); On 2015/09/15 14:15:50, Thomas Greiner wrote: > On 2015/09/15 12:43:31, saroyanm wrote: > > On 2015/09/15 11:19:32, Thomas Greiner wrote: > > > Two more issues: > > > > > > 1) According to MDN Safari 5.1 requires `-webkit-xxx` but without support > for > > > the `to xxx` syntax. > > > 2) It should be `-webkit-linear-gradient` and not `-webkit-gradient`. > > > > Done. > > Nope, while the `to X` syntax defines to which point a gradient should flow, the > old syntax defines where it originates. Therefore you need to either change > "top" to "bottom" or reverse the order of the color values. > > See also > https://developer.mozilla.org/en-US/docs/Web/CSS/linear-gradient#History_of_t... > > "Unfortunately, the addition of the <angle> values to the syntax introduced an > incoherence: the angle indicates a destination, but the keywords indicate a > starting point.# This was fixed by a new syntax where the keyword are directions > too, and preceded by the to keyword." Good to know, thanks. Made it somehow consistent with other implementation on the page, not sure whether make to also remove -moz prefixes from similar implementation of toggle buttons styles on line 663 and 667.
https://codereview.adblockplus.org/29326238/diff/29327704/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29327704/skin/firstRun.css#n... skin/firstRun.css:242: html[dir="rtl"] #general > div:not(:first-child) The second selector is redundant. https://codereview.adblockplus.org/29326238/diff/29327704/skin/firstRun.css#n... skin/firstRun.css:247: #general > div:not(:first-child) This can be merged with the rules above.
Patch uploaded. https://codereview.adblockplus.org/29326238/diff/29327704/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29327704/skin/firstRun.css#n... skin/firstRun.css:242: html[dir="rtl"] #general > div:not(:first-child) On 2015/09/16 07:38:24, Sebastian Noack wrote: > The second selector is redundant. The problem is that the: html[dir="rtl"] #general > div:not(:first-child) rule override this one if I'll not use more specific selector https://codereview.adblockplus.org/29326238/diff/29327704/skin/firstRun.css#n... skin/firstRun.css:247: #general > div:not(:first-child) On 2015/09/16 07:38:24, Sebastian Noack wrote: > This can be merged with the rules above. The rule above is used to remove the border from both side of the container while this is for adding border on top, but actually this selector missing a rule for "rtl" direction, which I've added.
https://codereview.adblockplus.org/29326238/diff/29327704/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29327704/skin/firstRun.css#n... skin/firstRun.css:184: text-align: center; You still need to align the text inside the button to the left because this will not only center the button but also its contents. https://codereview.adblockplus.org/29326238/diff/29327704/skin/firstRun.css#n... skin/firstRun.css:200: background: linear-gradient(to bottom, #6b92be, #294e76); This line should've stayed unchanged based on what I said in my previous comment. https://codereview.adblockplus.org/29326238/diff/29327985/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29327985/skin/firstRun.css#n... skin/firstRun.css:247: html[dir="rtl"] #general > div:not(:first-child) Text direction is irrelevant here because you're not making any left/right-specific changes here.
On 2015/09/16 10:09:20, Thomas Greiner wrote: > https://codereview.adblockplus.org/29326238/diff/29327704/skin/firstRun.css > File skin/firstRun.css (right): > > https://codereview.adblockplus.org/29326238/diff/29327704/skin/firstRun.css#n... > skin/firstRun.css:184: text-align: center; > You still need to align the text inside the button to the left because this will > not only center the button but also its contents. > > https://codereview.adblockplus.org/29326238/diff/29327704/skin/firstRun.css#n... > skin/firstRun.css:200: background: linear-gradient(to bottom, #6b92be, #294e76); > This line should've stayed unchanged based on what I said in my previous > comment. > > https://codereview.adblockplus.org/29326238/diff/29327985/skin/firstRun.css > File skin/firstRun.css (right): > > https://codereview.adblockplus.org/29326238/diff/29327985/skin/firstRun.css#n... > skin/firstRun.css:247: html[dir="rtl"] #general > div:not(:first-child) > Text direction is irrelevant here because you're not making any > left/right-specific changes here. Here you go: #general > div { border: dashed 0 #969085; -webkit-border-start-width: 1px; -moz-border-start-width: 1px; } #general > div:first-child { border: none; } @media (max-width: 960px) { #general > div { border-width: 1px 0 0; } }
https://codereview.adblockplus.org/29326238/diff/29327704/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29327704/skin/firstRun.css#n... skin/firstRun.css:184: text-align: center; On 2015/09/16 10:09:20, Thomas Greiner wrote: > You still need to align the text inside the button to the left because this will > not only center the button but also its contents. Done. https://codereview.adblockplus.org/29326238/diff/29327704/skin/firstRun.css#n... skin/firstRun.css:200: background: linear-gradient(to bottom, #6b92be, #294e76); On 2015/09/16 10:09:20, Thomas Greiner wrote: > This line should've stayed unchanged based on what I said in my previous > comment. Done. https://codereview.adblockplus.org/29326238/diff/29327985/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29327985/skin/firstRun.css#n... skin/firstRun.css:247: html[dir="rtl"] #general > div:not(:first-child) On 2015/09/16 10:09:20, Thomas Greiner wrote: > Text direction is irrelevant here because you're not making any > left/right-specific changes here. Done. https://codereview.adblockplus.org/29326238/diff/29327991/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29327991/skin/firstRun.css#n... skin/firstRun.css:242: #content #general > div:not(:first-child) I'm using #content for css Specificity, becuase html[dir=rtl] usually override the rules.
On 2015/09/16 10:16:33, Sebastian Noack wrote: > On 2015/09/16 10:09:20, Thomas Greiner wrote: > > https://codereview.adblockplus.org/29326238/diff/29327704/skin/firstRun.css > > File skin/firstRun.css (right): > > > > > https://codereview.adblockplus.org/29326238/diff/29327704/skin/firstRun.css#n... > > skin/firstRun.css:184: text-align: center; > > You still need to align the text inside the button to the left because this > will > > not only center the button but also its contents. > > > > > https://codereview.adblockplus.org/29326238/diff/29327704/skin/firstRun.css#n... > > skin/firstRun.css:200: background: linear-gradient(to bottom, #6b92be, > #294e76); > > This line should've stayed unchanged based on what I said in my previous > > comment. > > > > https://codereview.adblockplus.org/29326238/diff/29327985/skin/firstRun.css > > File skin/firstRun.css (right): > > > > > https://codereview.adblockplus.org/29326238/diff/29327985/skin/firstRun.css#n... > > skin/firstRun.css:247: html[dir="rtl"] #general > div:not(:first-child) > > Text direction is irrelevant here because you're not making any > > left/right-specific changes here. > > Here you go: > > #general > div > { > border: dashed 0 #969085; > -webkit-border-start-width: 1px; > -moz-border-start-width: 1px; > } > > #general > div:first-child > { > border: none; > } > > @media (max-width: 960px) > { > #general > div > { > border-width: 1px 0 0; > } > } -webkit-border-start-width: 1px; -moz-border-start-width: 1px; What about support of this syntax ?
On 2015/09/16 11:11:03, saroyanm wrote: > -webkit-border-start-width: 1px; > -moz-border-start-width: 1px; > > What about support of this syntax ? This syntax is quite old and supported in all supported Chrome, Firefox and Safari versions. Note that we already use -prefix-border-start-style on the first run page. Generally, it's preferable to refer to start/end using these properties rather than hardcoding left/right with hacks for rtl languages.
On 2015/09/16 11:14:37, Sebastian Noack wrote: > On 2015/09/16 11:11:03, saroyanm wrote: > > -webkit-border-start-width: 1px; > > -moz-border-start-width: 1px; > > > > What about support of this syntax ? > > This syntax is quite old and supported in all supported Chrome, Firefox and > Safari versions. Note that we already use -prefix-border-start-style on the > first run page. Generally, it's preferable to refer to start/end using these > properties rather than hardcoding left/right with hacks for rtl languages. According to MDN -moz-border-start-width is Superseded by the standard version border-inline-start-width, which is only supported in FF 41+ https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Mozilla_Extensions https://developer.mozilla.org/en-US/docs/Web/CSS/border-inline-start-width
On 2015/09/16 11:22:14, saroyanm wrote: > On 2015/09/16 11:14:37, Sebastian Noack wrote: > > On 2015/09/16 11:11:03, saroyanm wrote: > > > -webkit-border-start-width: 1px; > > > -moz-border-start-width: 1px; > > > > > > What about support of this syntax ? > > > > This syntax is quite old and supported in all supported Chrome, Firefox and > > Safari versions. Note that we already use -prefix-border-start-style on the > > first run page. Generally, it's preferable to refer to start/end using these > > properties rather than hardcoding left/right with hacks for rtl languages. > > According to MDN -moz-border-start-width is Superseded by the standard version > border-inline-start-width, which is only supported in FF 41+ > https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Mozilla_Extensions > https://developer.mozilla.org/en-US/docs/Web/CSS/border-inline-start-width Good that this finally made it into the standard. So just use the unprefixed version in addition, as we do for other new properties as well: -webkit-border-start-width: 1px; -moz-border-start-width: 1px; border-inline-start-width: 1px;
On 2015/09/16 11:29:49, Sebastian Noack wrote: > On 2015/09/16 11:22:14, saroyanm wrote: > > On 2015/09/16 11:14:37, Sebastian Noack wrote: > > > On 2015/09/16 11:11:03, saroyanm wrote: > > > > -webkit-border-start-width: 1px; > > > > -moz-border-start-width: 1px; > > > > > > > > What about support of this syntax ? > > > > > > This syntax is quite old and supported in all supported Chrome, Firefox and > > > Safari versions. Note that we already use -prefix-border-start-style on the > > > first run page. Generally, it's preferable to refer to start/end using these > > > properties rather than hardcoding left/right with hacks for rtl languages. > > > > According to MDN -moz-border-start-width is Superseded by the standard version > > border-inline-start-width, which is only supported in FF 41+ > > https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Mozilla_Extensions > > https://developer.mozilla.org/en-US/docs/Web/CSS/border-inline-start-width > > Good that this finally made it into the standard. So just use the unprefixed > version in addition, as we do for other new properties as well: > > -webkit-border-start-width: 1px; > -moz-border-start-width: 1px; > border-inline-start-width: 1px; I can't find anywhere support information, doesn't looks to be well supported, Can you please refer to some support information ? I can't find anything.
On 2015/09/16 11:37:37, saroyanm wrote: > On 2015/09/16 11:29:49, Sebastian Noack wrote: > > Good that this finally made it into the standard. So just use the unprefixed > > version in addition, as we do for other new properties as well: > > > > -webkit-border-start-width: 1px; > > -moz-border-start-width: 1px; > > border-inline-start-width: 1px; > > I can't find anywhere support information, doesn't looks to be well supported, > Can you please refer to some support information ? I can't find anything. I'm not aware of any reference documenting the browser compatibility of the prefixed versions either. But back then when we did start using -prefix-border-start I checked myself that it is supported by all versions of Chrome, Firefox and Safari that we support.
On 2015/09/16 11:41:03, Sebastian Noack wrote: > On 2015/09/16 11:37:37, saroyanm wrote: > > On 2015/09/16 11:29:49, Sebastian Noack wrote: > > > Good that this finally made it into the standard. So just use the unprefixed > > > version in addition, as we do for other new properties as well: > > > > > > -webkit-border-start-width: 1px; > > > -moz-border-start-width: 1px; > > > border-inline-start-width: 1px; > > > > I can't find anywhere support information, doesn't looks to be well supported, > > Can you please refer to some support information ? I can't find anything. > > I'm not aware of any reference documenting the browser compatibility of the > prefixed versions either. But back then when we did start using > -prefix-border-start I checked myself that it is supported by all versions of > Chrome, Firefox and Safari that we support. I'm not sure if it's reliable to use property that is not documented anywhere, it doesn't looks to be supported anymore. I have assumption that it has been deprecated from some point, so I'd be a fan of using "border-inline-start-width" as soon it will start to be supported by our supported browsers. What you think @Thomas ?
On 2015/09/16 11:46:26, saroyanm wrote:
> I'm not sure if it's reliable to use property that is not documented anywhere,
> it doesn't looks to be supported anymore. I have assumption that it has been
> deprecated from some point, so I'd be a fan of using
"border-inline-start-width"
> as soon it will start to be supported by our supported browsers.
I strongly disagree. We already use -prefix-border-start-style and it is used to
work reliable. IMO it's also certainly better than using [dir=rtl] hacks
involving a fair amount of duplication. Also with your argumentation we
shouldn't use any prefixed property. So I don't see what's the deal of using
border-inline-start-width with -webkit-border-start-width and
-moz-border-start-width as fallback for legacy browsers here. But most
importantly, it's way more maintainable to write dir-agnostic styles. To
demonstrate how significant that is, here is the code that you could remove:
#general > div:not(:first-child)
{
border-left: dashed 1px #969085;
}
html[dir="rtl"] #general > div:not(:first-child)
{
border-left: none;
border-right: dashed 1px #969085;
}
@media (max-width: 960px)
{
#general > div:not(:first-child),
html[dir="rtl"] #general > div:not(:first-child)
{
border: none;
}
#general > div:not(:first-child)
{
border-top: dashed 1px #969085;
}
}
If you use following code instead:
#general > div
{
border: dashed 0 #969085;
-webkit-border-start-width: 1px;
-moz-border-start-width: 1px;
border-inline-start-width: 1px;
}
#general > div:first-child
{
border: none;
}
@media (max-width: 960px)
{
#general > div
{
border-width: 1px 0 0;
}
}
It's not only way shorter, but also way simpler and requires almost no
duplication.
On 2015/09/16 12:04:00, Sebastian Noack wrote:
> On 2015/09/16 11:46:26, saroyanm wrote:
> > I'm not sure if it's reliable to use property that is not documented
anywhere,
> > it doesn't looks to be supported anymore. I have assumption that it has been
> > deprecated from some point, so I'd be a fan of using
> "border-inline-start-width"
> > as soon it will start to be supported by our supported browsers.
>
> I strongly disagree. We already use -prefix-border-start-style and it is used
to
> work reliable. IMO it's also certainly better than using [dir=rtl] hacks
> involving a fair amount of duplication. Also with your argumentation we
> shouldn't use any prefixed property. So I don't see what's the deal of using
> border-inline-start-width with -webkit-border-start-width and
> -moz-border-start-width as fallback for legacy browsers here. But most
> importantly, it's way more maintainable to write dir-agnostic styles. To
> demonstrate how significant that is, here is the code that you could remove:
>
> #general > div:not(:first-child)
> {
> border-left: dashed 1px #969085;
> }
>
> html[dir="rtl"] #general > div:not(:first-child)
> {
> border-left: none;
> border-right: dashed 1px #969085;
> }
>
> @media (max-width: 960px)
> {
> #general > div:not(:first-child),
> html[dir="rtl"] #general > div:not(:first-child)
> {
> border: none;
> }
>
> #general > div:not(:first-child)
> {
> border-top: dashed 1px #969085;
> }
> }
>
> If you use following code instead:
>
> #general > div
> {
> border: dashed 0 #969085;
> -webkit-border-start-width: 1px;
> -moz-border-start-width: 1px;
> border-inline-start-width: 1px;
> }
>
> #general > div:first-child
> {
> border: none;
> }
>
> @media (max-width: 960px)
> {
> #general > div
> {
> border-width: 1px 0 0;
> }
> }
>
> It's not only way shorter, but also way simpler and requires almost no
> duplication.
According to caniuse.com (see http://caniuse.com/#feat=css-logical-props) none
of the browsers supports this unofficial standard in its entirety but using
prefixes we should be able to use them for what we need without any problems, it
seems.
https://codereview.adblockplus.org/29326238/diff/29327991/skin/firstRun.css
File skin/firstRun.css (right):
https://codereview.adblockplus.org/29326238/diff/29327991/skin/firstRun.css#n...
skin/firstRun.css:202: text-align: left;
What about RTL languages?
https://codereview.adblockplus.org/29326238/diff/29327991/skin/firstRun.css#n...
skin/firstRun.css:247: #content #general > div:not(:first-child)
You're using the same selector as the block above so please merge those two
blocks to avoid duplication.
#content #general > div:not(:first-child)
{
border: none;
border-top: dashed 1px #969085;
}
https://codereview.adblockplus.org/29326238/diff/29327991/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29327991/skin/firstRun.css#n... skin/firstRun.css:202: text-align: left; On 2015/09/16 12:28:12, Thomas Greiner wrote: > What about RTL languages? Well spotted. It should be |text-algin: start|. https://codereview.adblockplus.org/29326238/diff/29327991/skin/firstRun.css#n... skin/firstRun.css:247: #content #general > div:not(:first-child) On 2015/09/16 12:28:12, Thomas Greiner wrote: > You're using the same selector as the block above so please merge those two > blocks to avoid duplication. > > #content #general > div:not(:first-child) > { > border: none; > border-top: dashed 1px #969085; > } This will be redundant, if you use the CSS I suggested.
New patch uploaded. https://codereview.adblockplus.org/29326238/diff/29327991/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29327991/skin/firstRun.css#n... skin/firstRun.css:202: text-align: left; On 2015/09/16 12:32:33, Sebastian Noack wrote: > On 2015/09/16 12:28:12, Thomas Greiner wrote: > > What about RTL languages? > > Well spotted. It should be |text-algin: start|. Done.
LGTM
One more issue I found while testing. https://codereview.adblockplus.org/29326238/diff/29328005/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29328005/skin/firstRun.css#n... skin/firstRun.css:205: display: inline-block; This causes the text inside to be moved below the logo when it is longer than the available width. Reverting back to `display: table-cell` appears to resolve this issue.
https://codereview.adblockplus.org/29326238/diff/29328005/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29326238/diff/29328005/skin/firstRun.css#n... skin/firstRun.css:205: display: inline-block; On 2015/09/16 14:09:08, Thomas Greiner wrote: > This causes the text inside to be moved below the logo when it is longer than > the available width. Reverting back to `display: table-cell` appears to resolve > this issue. Thanks for noticing, Done.
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
