Issue 29655630: Issue 5873 - Show original subscription title in languages table (Closed)
DescriptionIssue 5873 - Show original subscription title in languages table
Patch Set 1 #
Total comments: 11
Patch Set 2 : #
Total comments: 8
Patch Set 3 : #
MessagesTotal messages: 8
@Ire can you please have a look when you have a chance.
Hey Manvel, comments below. https://codereview.adblockplus.org/29655630/diff/29655631/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29655630/diff/29655631/desktop-options.htm... desktop-options.html:137: <label> Using the <label> element here (and below) may not be appropriate. The <label> element should have a control associated with it, either through the `for` attribute or the control nested within it. https://codereview.adblockplus.org/29655630/diff/29655631/desktop-options.htm... desktop-options.html:377: <label> As I mentioned above the <label> here isn't appropriate because it's nested the wrong way around. In this case I think a simple <span> will work since the label is already the content of the <button> https://codereview.adblockplus.org/29655630/diff/29655631/desktop-options.htm... desktop-options.html:379: <em>(<span data-display="originalTitle"></span>)</em> Should this be an `<em>` element? In the spec screenshots it doesn't appear italic https://codereview.adblockplus.org/29655630/diff/29655631/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29655630/diff/29655631/desktop-options.js#... desktop-options.js:93: { NIT: I think this was cleaner without the nested `if`s https://codereview.adblockplus.org/29655630/diff/29655631/desktop-options.js#... desktop-options.js:95: if (item.originalTitle && item.originalTitle.indexOf("+EasyList") >= 0) NIT: I don't know if it actually makes a difference, but I always prefer checking for `> -1` rather than `>= 0`.
Thanks Ire, new patch uploaded. https://codereview.adblockplus.org/29655630/diff/29655631/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29655630/diff/29655631/desktop-options.htm... desktop-options.html:137: <label> On 2018/01/04 08:32:52, ire wrote: > Using the <label> element here (and below) may not be appropriate. The <label> > element should have a control associated with it, either through the `for` > attribute or the control nested within it. Agree, done. https://codereview.adblockplus.org/29655630/diff/29655631/desktop-options.htm... desktop-options.html:377: <label> On 2018/01/04 08:32:52, ire wrote: > As I mentioned above the <label> here isn't appropriate because it's nested the > wrong way around. In this case I think a simple <span> will work since the label > is already the content of the <button> Agree, done. https://codereview.adblockplus.org/29655630/diff/29655631/desktop-options.htm... desktop-options.html:379: <em>(<span data-display="originalTitle"></span>)</em> On 2018/01/04 08:32:52, ire wrote: > Should this be an `<em>` element? In the spec screenshots it doesn't appear > italic Initially I though we would like to <em> as stress emphasis, but I don't have strong opinion regarding that, though I can style it non Italic, anyway having span should also be fine for now. I'll go for that. Done. https://codereview.adblockplus.org/29655630/diff/29655631/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29655630/diff/29655631/desktop-options.js#... desktop-options.js:93: { On 2018/01/04 08:32:52, ire wrote: > NIT: I think this was cleaner without the nested `if`s Yes, previously logic was bit different: Previously, we were defining which attribute value to use, while with new requirements we have an exceptional case, so I implemented: if useSpacialization then use it and add English if aplicable. While previously it was along the lines: If useSpacialization then use it, if useOriginalTitle then use it. I think you are right and my approach in general is wrong that why it doesn't feel to be correct. I should have separated special case handling from core "collection" functionality. I did create a special case separate function, so we can specify custom function for every special "_getItemTitle" case, so it will still fit into the logic of collections. I hope this implementation is more clear. https://codereview.adblockplus.org/29655630/diff/29655631/desktop-options.js#... desktop-options.js:95: if (item.originalTitle && item.originalTitle.indexOf("+EasyList") >= 0) On 2018/01/04 08:32:52, ire wrote: > NIT: I don't know if it actually makes a difference, but I always prefer > checking for `> -1` rather than `>= 0`. Done.
LGTM + NIT https://codereview.adblockplus.org/29655630/diff/29655631/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29655630/diff/29655631/desktop-options.js#... desktop-options.js:93: { On 2018/01/04 21:28:52, saroyanm wrote: > On 2018/01/04 08:32:52, ire wrote: > > NIT: I think this was cleaner without the nested `if`s > > Yes, previously logic was bit different: > Previously, we were defining which attribute value to use, while with new > requirements we have an exceptional case, so I implemented: > if useSpacialization then use it and add English if aplicable. > While previously it was along the lines: > If useSpacialization then use it, if useOriginalTitle then use it. > > I think you are right and my approach in general is wrong that why it doesn't > feel to be correct. I should have separated special case handling from core > "collection" functionality. > > I did create a special case separate function, so we can specify custom function > for every special "_getItemTitle" case, so it will still fit into the logic of > collections. > > I hope this implementation is more clear. Ack. Yes it is more clear, thanks! https://codereview.adblockplus.org/29655630/diff/29656607/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29655630/diff/29656607/skin/desktop-option... skin/desktop-options.css:1034: .table.list li .dim NIT: I don't think the classname "dim" is very clear (I don't know what it means actually) https://codereview.adblockplus.org/29655630/diff/29656607/skin/desktop-option... skin/desktop-options.css:1036: color: #BBB; This is not relative for this issue, but the contrast ratio for this colour against the white background is only 1.2 . Please bring this up with Jeen separately
https://codereview.adblockplus.org/29655630/diff/29656607/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29655630/diff/29656607/skin/desktop-option... skin/desktop-options.css:1034: .table.list li .dim On 2018/01/05 09:23:46, ire wrote: > NIT: I don't think the classname "dim" is very clear (I don't know what it means > actually) I used it as current meaning -> "make or become less bright or distinct". I still can use "EM" instead and style it not to look Italic, if you agree with my previous comments, that EM can fit here as well ? I can use also <small>, but that will require font-size adjustments which I would avoid. Do you have a suggestion ? Or strong preference ? https://codereview.adblockplus.org/29655630/diff/29656607/skin/desktop-option... skin/desktop-options.css:1036: color: #BBB; On 2018/01/05 09:23:45, ire wrote: > This is not relative for this issue, but the contrast ratio for this colour > against the white background is only 1.2 . Please bring this up with Jeen > separately Right, we have an issue already filed for that -> https://gitlab.com/eyeo/spec/issues/70
https://codereview.adblockplus.org/29655630/diff/29656607/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29655630/diff/29656607/skin/desktop-option... skin/desktop-options.css:1034: .table.list li .dim On 2018/01/05 12:02:17, saroyanm wrote: > On 2018/01/05 09:23:46, ire wrote: > > NIT: I don't think the classname "dim" is very clear (I don't know what it > means > > actually) > I used it as current meaning -> "make or become less bright or distinct". > > I still can use "EM" instead and style it not to look Italic, if you agree with > my previous comments, that EM can fit here as well ? > > I can use also <small>, but that will require font-size adjustments which I > would avoid. > > Do you have a suggestion ? Or strong preference ? Oh haha I assumed it was an acronym for something, not literally the word "dim". Your class name makes sense then, I was the one that misunderstood. Although, a common class name for the "dimmed" items is "muted", so perhaps you can use that instead? You could also stick with dim, or even call it "dimmed" so its more clear that you're using the word and it's not an acronym. https://codereview.adblockplus.org/29655630/diff/29656607/skin/desktop-option... skin/desktop-options.css:1036: color: #BBB; On 2018/01/05 12:02:17, saroyanm wrote: > On 2018/01/05 09:23:45, ire wrote: > > This is not relative for this issue, but the contrast ratio for this colour > > against the white background is only 1.2 . Please bring this up with Jeen > > separately > > Right, we have an issue already filed for that -> > https://gitlab.com/eyeo/spec/issues/70 Thank you
https://codereview.adblockplus.org/29655630/diff/29656607/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29655630/diff/29656607/skin/desktop-option... skin/desktop-options.css:1034: .table.list li .dim On 2018/01/08 10:38:35, ire wrote: > On 2018/01/05 12:02:17, saroyanm wrote: > > On 2018/01/05 09:23:46, ire wrote: > > > NIT: I don't think the classname "dim" is very clear (I don't know what it > > means > > > actually) > > I used it as current meaning -> "make or become less bright or distinct". > > > > I still can use "EM" instead and style it not to look Italic, if you agree > with > > my previous comments, that EM can fit here as well ? > > > > I can use also <small>, but that will require font-size adjustments which I > > would avoid. > > > > Do you have a suggestion ? Or strong preference ? > > Oh haha I assumed it was an acronym for something, not literally the word "dim". > Your class name makes sense then, I was the one that misunderstood. Although, a > common class name for the "dimmed" items is "muted", so perhaps you can use that > instead? > > You could also stick with dim, or even call it "dimmed" so its more clear that > you're using the word and it's not an acronym. I used Dim, as an adjective -> https://dictionary.cambridge.org/dictionary/english/dim ex.: The lamp gave out a dim light. But I changed it to dimmed, so it will be more obvious what it was meant here.
LGTM (again) https://codereview.adblockplus.org/29655630/diff/29656607/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29655630/diff/29656607/skin/desktop-option... skin/desktop-options.css:1034: .table.list li .dim On 2018/01/08 12:53:57, saroyanm wrote: > On 2018/01/08 10:38:35, ire wrote: > > On 2018/01/05 12:02:17, saroyanm wrote: > > > On 2018/01/05 09:23:46, ire wrote: > > > > NIT: I don't think the classname "dim" is very clear (I don't know what it > > > means > > > > actually) > > > I used it as current meaning -> "make or become less bright or distinct". > > > > > > I still can use "EM" instead and style it not to look Italic, if you agree > > with > > > my previous comments, that EM can fit here as well ? > > > > > > I can use also <small>, but that will require font-size adjustments which I > > > would avoid. > > > > > > Do you have a suggestion ? Or strong preference ? > > > > Oh haha I assumed it was an acronym for something, not literally the word > "dim". > > Your class name makes sense then, I was the one that misunderstood. Although, > a > > common class name for the "dimmed" items is "muted", so perhaps you can use > that > > instead? > > > > You could also stick with dim, or even call it "dimmed" so its more clear that > > you're using the word and it's not an acronym. > > I used Dim, as an adjective -> > https://dictionary.cambridge.org/dictionary/english/dim > > ex.: The lamp gave out a dim light. > > But I changed it to dimmed, so it will be more obvious what it was meant here. Yes I get that now. Thank you! 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
