Issue 29811582: Fixes #88 - Made short policy lists inline and semicolon separated on privacy page (Closed)
|
Created:
June 20, 2018, 3:25 p.m. by juliandoucette Modified:
June 22, 2018, 4:04 p.m. Reviewers:
ire Base URL:
https://hg.adblockplus.org/web.adblockplus.org Visibility:
Public. |
DescriptionFixes #88 - Made short policy lists inline and semicolon separated on privacy page
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addressed #5 #MessagesTotal messages: 8
On 2018/06/20 15:25:58, juliandoucette wrote: Detail: The default.css update wasn't strictly necessary for this change as I changed the way horizontal-list works on this page specifically. I included https://codereview.adblockplus.org/29811579 in said update. And I will push it (the updated to default.css on adblockplus.org) in a separate commit. I included it in this review to kill two birds with one stone if possible - since the change just mentioned (by review link) was mostly intended for the privacy page.
On 2018/06/20 15:28:40, juliandoucette wrote: > On 2018/06/20 15:25:58, juliandoucette wrote: > > Detail: The default.css update wasn't strictly necessary for this change as I > changed the way horizontal-list works on this page specifically. I included > https://codereview.adblockplus.org/29811579 in said update. And I will push it > (the updated to default.css on http://adblockplus.org) in a separate commit. I included > it in this review to kill two birds with one stone if possible - since the > change just mentioned (by review link) was mostly intended for the privacy page. Thanks for the context, makes sense https://codereview.adblockplus.org/29811582/diff/29811583/pages/privacy.md File pages/privacy.md (right): https://codereview.adblockplus.org/29811582/diff/29811583/pages/privacy.md#ne... pages/privacy.md:7: .horizontal-list.semicolon-separated See my comment below about the classes being applied to the wrong element. These styles won't be needed if these classes were applied to the <ul> itself. https://codereview.adblockplus.org/29811582/diff/29811583/pages/privacy.md#ne... pages/privacy.md:19: .horizontal-list.semicolon-separated li These styles are already covered by the .horizontal-list class https://codereview.adblockplus.org/29811582/diff/29811583/pages/privacy.md#ne... pages/privacy.md:55: {: .horizontal-list .semicolon-separated } NIT: These classes are being applied to the <li>, not the <ul> we're actually trying to target. I think it's a bit weird to have the `.horizontal-list .semicolon-separated` being applied to the element that isn't actually a horizontal list or semicolon separated.
https://codereview.adblockplus.org/29811582/diff/29811583/pages/privacy.md File pages/privacy.md (right): https://codereview.adblockplus.org/29811582/diff/29811583/pages/privacy.md#ne... pages/privacy.md:55: {: .horizontal-list .semicolon-separated } On 2018/06/20 18:33:19, ire wrote: > NIT: These classes are being applied to the <li>, not the <ul> we're actually > trying to target. I think it's a bit weird to have the `.horizontal-list > .semicolon-separated` being applied to the element that isn't actually a > horizontal list or semicolon separated. Ack. I'm trying to keep this page in markdown and apply this class the "markdown way". This is the best way I've come up with. But I'm open to suggestion? e.g. I could rename the class to .has-horizontal-list or similar as I am targeting the child list?
On 2018/06/21 12:17:59, juliandoucette wrote: > https://codereview.adblockplus.org/29811582/diff/29811583/pages/privacy.md > File pages/privacy.md (right): > > https://codereview.adblockplus.org/29811582/diff/29811583/pages/privacy.md#ne... > pages/privacy.md:55: {: .horizontal-list .semicolon-separated } > On 2018/06/20 18:33:19, ire wrote: > > NIT: These classes are being applied to the <li>, not the <ul> we're actually > > trying to target. I think it's a bit weird to have the `.horizontal-list > > .semicolon-separated` being applied to the element that isn't actually a > > horizontal list or semicolon separated. > > Ack. I'm trying to keep this page in markdown and apply this class the "markdown > way". This is the best way I've come up with. But I'm open to suggestion? > > e.g. I could rename the class to .has-horizontal-list or similar as I am > targeting the child list? Since there is no way to add a class to the ul without switching to HTML, I think your suggestion to rename the class to something along the lines of .has-horizontal-list would be the best solution.
On 2018/06/21 16:05:56, ire wrote: > Since there is no way to add a class to the ul without switching to HTML, I > think your suggestion to rename the class to something along the lines of > .has-horizontal-list would be the best solution. Done. Detail: I removed the `.semicolon-separated` modifier and the default.css update because we're no longer using anything from website-defaults for this list.
On 2018/06/22 11:31:14, juliandoucette wrote: > On 2018/06/21 16:05:56, ire wrote: > > Since there is no way to add a class to the ul without switching to HTML, I > > think your suggestion to rename the class to something along the lines of > > .has-horizontal-list would be the best solution. > > Done. > > Detail: I removed the `.semicolon-separated` modifier and the default.css update > because we're no longer using anything from website-defaults for this list. Ack. LGTM
On 2018/06/22 16:01:37, ire wrote: > Ack. LGTM https://hg.adblockplus.org/web.adblockplus.org/rev/16fffa04f302 
|
