Issue 29328733: Issue 2018 - Optimized first-run page for smaller screens (Closed)
|
Created:
Sept. 30, 2015, 5:21 p.m. by Thomas Greiner Modified:
Oct. 12, 2015, 9:52 a.m. CC:
Felix Dahlke Visibility:
Public. |
DescriptionThis review also contains minor CSS coding style inconsistency improvements and a fix for feature text in RTL environment.
Patch Set 1 #
Total comments: 3
Patch Set 2 : #
Total comments: 11
Patch Set 3 : #
Total comments: 5
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
MessagesTotal messages: 12
https://codereview.adblockplus.org/29328733/diff/29328734/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29328733/diff/29328734/skin/firstRun.css#n... skin/firstRun.css:309: margin-start: 20px; It seems there isn't an unprefixed margin-start, but margin-inline-start and margin-block-start. https://developer.mozilla.org/en-US/docs/Web/CSS/margin-inline-start https://developer.mozilla.org/en-US/docs/Web/CSS/margin-block-start Same for other -start/-end properties.
https://codereview.adblockplus.org/29328733/diff/29328734/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29328733/diff/29328734/skin/firstRun.css#n... skin/firstRun.css:309: margin-start: 20px; On 2015/10/05 12:23:56, Sebastian Noack wrote: > It seems there isn't an unprefixed margin-start, but margin-inline-start and > margin-block-start. > > https://developer.mozilla.org/en-US/docs/Web/CSS/margin-inline-start > https://developer.mozilla.org/en-US/docs/Web/CSS/margin-block-start > > Same for other -start/-end properties. Both are part of experimental API. Once we implemented border-inline-start-width in this file, but didn't consider them of being experimental, so I think we need to go with prefixed versions only, there is also discussion regarding that in previous review for adblockplusui module: https://codereview.adblockplus.org/29323156/diff/29328570/skin/options.css#ne...
https://codereview.adblockplus.org/29328733/diff/29328734/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29328733/diff/29328734/skin/firstRun.css#n... skin/firstRun.css:309: margin-start: 20px; On 2015/10/05 12:51:10, saroyanm wrote: > On 2015/10/05 12:23:56, Sebastian Noack wrote: > > It seems there isn't an unprefixed margin-start, but margin-inline-start and > > margin-block-start. > > > > https://developer.mozilla.org/en-US/docs/Web/CSS/margin-inline-start > > https://developer.mozilla.org/en-US/docs/Web/CSS/margin-block-start > > > > Same for other -start/-end properties. > > Both are part of experimental API. Once we implemented border-inline-start-width > in this file, but didn't consider them of being experimental, so I think we need > to go with prefixed versions only, there is also discussion regarding that in > previous review for adblockplusui module: > https://codereview.adblockplus.org/29323156/diff/29328570/skin/options.css#ne... I did a bit of research on that since the situation is more convoluted than I thought. Apparently, the prefixed versions of `margin-{start,end}` were dropped judging from the 404 page on MDN (https://developer.mozilla.org/en-US/docs/Talk%3ACSS/-moz-margin-start). However, the `margin-{block,inline}-{start,end}` properties are currently only supported in Firefox according to MDN and I can confirm that using those shows the "Unknown property name" warning in Chrome's Developer Tools. Therefore it seems that were stuck with using the prefixed versions of `margin-{start,end}`.
I'm not sure if we need to also fix the dashed border issue in share section as well while you made a remark in #3089, but that can be done in #3089, so will leave up to you. https://codereview.adblockplus.org/29328733/diff/29328845/firstRun.html File firstRun.html (right): https://codereview.adblockplus.org/29328733/diff/29328845/firstRun.html#newco... firstRun.html:23: <meta name="viewport" content="width=device-width, initial-scale=1"> Nit: Please also close the tag. https://codereview.adblockplus.org/29328733/diff/29328845/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29328733/diff/29328845/skin/firstRun.css#n... skin/firstRun.css:82: padding-top: 25px; Should we need to use shorthand in this case ? Have doubt, what you think @Thomas ? https://codereview.adblockplus.org/29328733/diff/29328845/skin/firstRun.css#n... skin/firstRun.css:102: h1, h2, h3 Nit: I think each selectors should go to it's own line in this case. https://codereview.adblockplus.org/29328733/diff/29328845/skin/firstRun.css#n... skin/firstRun.css:123: margin-top: 0px; Nit: I think you can use shorthand here. https://codereview.adblockplus.org/29328733/diff/29328845/skin/firstRun.css#n... skin/firstRun.css:151: padding-left: 0px; Nit: I think you can use shorthand here. https://codereview.adblockplus.org/29328733/diff/29328845/skin/firstRun.css#n... skin/firstRun.css:547: margin-top: 32px; Why shorthand was removed ? Will stop commenting about shorthand properties from here. https://codereview.adblockplus.org/29328733/diff/29328845/skin/firstRun.css#n... skin/firstRun.css:702: #can-do-more .feature h3 can be merged with "#can-do-more"
https://codereview.adblockplus.org/29328733/diff/29328845/firstRun.html File firstRun.html (right): https://codereview.adblockplus.org/29328733/diff/29328845/firstRun.html#newco... firstRun.html:23: <meta name="viewport" content="width=device-width, initial-scale=1"> On 2015/10/05 17:18:55, saroyanm wrote: > Nit: Please also close the tag. Done. https://codereview.adblockplus.org/29328733/diff/29328845/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29328733/diff/29328845/skin/firstRun.css#n... skin/firstRun.css:102: h1, h2, h3 On 2015/10/05 17:18:55, saroyanm wrote: > Nit: I think each selectors should go to it's own line in this case. Done. https://codereview.adblockplus.org/29328733/diff/29328845/skin/firstRun.css#n... skin/firstRun.css:123: margin-top: 0px; On 2015/10/05 17:18:55, saroyanm wrote: > Nit: I think you can use shorthand here. The problem with using shorthand properties here is that we'd override `margin-left` and `margin-right` which could've been specified elsewhere even though we don't really care about what values they have. Probably makes sense to take that into consideration for #3158. Shorthand properties make sense if they don't force you to override stuff that you don't need to. Otherwise they may lead to duplication of values. Example: h3 { margin-left: 10px; } header h3 { /* using shorthand: incorrect */ margin: 0px 0px 10px 0px; /* using shorthand: correct */ margin: 0px 0px 10px 10px; /* not using shorthand */ margin-bottom: 10px; } Problem: The correct shorthand requires duplication of the defined value above. https://codereview.adblockplus.org/29328733/diff/29328845/skin/firstRun.css#n... skin/firstRun.css:702: #can-do-more .feature h3 On 2015/10/05 17:18:55, saroyanm wrote: > can be merged with "#can-do-more" Done.
https://codereview.adblockplus.org/29328733/diff/29328857/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29328733/diff/29328857/skin/firstRun.css#n... skin/firstRun.css:153: padding-left: 0px; I can see that here you don't want to overwrite rule for "section" element, but in this case I think the exception is "section" with ID "share", because currently you are duplicating this style to another element with ID "can-do-more" make sense to have: section { padding: 40px 0px; } #share { padding-left: 100px; padding-right: 100px; } Or all sections can have equal left-right paddings and do not overwrite them. "section" elements that we are using to show error don't have heading and I'm not sure if we should consider them as sections, maybe make sense to use DIV instead ? Because seems like we are using them as section because of styling. So I'll suggest, or to make consistent changes, or revert this change and handle similar styling issue in separate ticket, while it's irrelevant here. https://codereview.adblockplus.org/29328733/diff/29328857/skin/firstRun.css#n... skin/firstRun.css:582: padding-left: 10px; What about RTL ? https://codereview.adblockplus.org/29328733/diff/29328857/skin/firstRun.css#n... skin/firstRun.css:592: padding-left: 30px; What about RTL ?
https://codereview.adblockplus.org/29328733/diff/29328857/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29328733/diff/29328857/skin/firstRun.css#n... skin/firstRun.css:153: padding-left: 0px; On 2015/10/07 10:15:07, saroyanm wrote: > I can see that here you don't want to overwrite rule for "section" element, but > in this case I think the exception is "section" with ID "share", because > currently you are duplicating this style to another element with ID > "can-do-more" make sense to have: > section > { > padding: 40px 0px; > } > > #share > { > padding-left: 100px; > padding-right: 100px; > } > > Or all sections can have equal left-right paddings and do not overwrite them. > > "section" elements that we are using to show error don't have heading and I'm > not sure if we should consider them as sections, maybe make sense to use DIV > instead ? Because seems like we are using them as section because of styling. > > So I'll suggest, or to make consistent changes, or revert this change and handle > similar styling issue in separate ticket, while it's irrelevant here. The values in `#can-do-more` were duplicates so I eliminated all of the redundancy that was going on there. https://codereview.adblockplus.org/29328733/diff/29328857/skin/firstRun.css#n... skin/firstRun.css:582: padding-left: 10px; On 2015/10/07 10:15:07, saroyanm wrote: > What about RTL ? Not everything needs to be inverted in an RTL environment. So the question we have to ask ourselves is: Does it make a difference whether that circle in the toggle button is on the right or left side? I don't think it does in this case. There's a nice blog post about that on Mozilla Hacks: https://hacks.mozilla.org/2015/09/building-rtl-aware-web-apps-and-websites-pa...
https://codereview.adblockplus.org/29328733/diff/29328905/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29328733/diff/29328905/skin/firstRun.css#n... skin/firstRun.css:145: max-width: 760px; You don't need to specify different max-width for different sections. Use border-box as box-sizing and keep 960px, in that case you don't need to have separate width values for sections.
https://codereview.adblockplus.org/29328733/diff/29328905/skin/firstRun.css File skin/firstRun.css (right): https://codereview.adblockplus.org/29328733/diff/29328905/skin/firstRun.css#n... skin/firstRun.css:145: max-width: 760px; On 2015/10/07 17:26:40, saroyanm wrote: > You don't need to specify different max-width for different sections. > Use border-box as box-sizing and keep 960px, in that case you don't need to have > separate width values for sections. Done. I'd suggest to tackle any other style oddities in this file that are not directly related to this specific change when applying the upcoming CSS coding style.
On 2015/10/08 14:59:54, Thomas Greiner wrote: > https://codereview.adblockplus.org/29328733/diff/29328905/skin/firstRun.css > File skin/firstRun.css (right): > > https://codereview.adblockplus.org/29328733/diff/29328905/skin/firstRun.css#n... > skin/firstRun.css:145: max-width: 760px; > On 2015/10/07 17:26:40, saroyanm wrote: > > You don't need to specify different max-width for different sections. > > Use border-box as box-sizing and keep 960px, in that case you don't need to > have > > separate width values for sections. > > Done. > > I'd suggest to tackle any other style oddities in this file that are not > directly related to this specific change when applying the upcoming CSS coding > style. LGTM
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
