Issue 29673576: Make new options page compatible with edge (Closed)
|
Created:
Jan. 18, 2018, 11:13 a.m. by a.giammarchi Modified:
Feb. 8, 2018, 3:38 p.m. Visibility:
Public. |
DescriptionDirectly from ticket https://issues.adblockplus.org/ticket/5813, I have changed few CSS things unavailable in Edge and took the opportunity to use native closest API where appropriate and double checked on VirtualBox Win10 + Edge that everything looks the same.
I am not 100% sure how to counter-verify that everything also works well but by clicking around "all the clickable" I think it's fine.
Patch Set 1 #
Total comments: 18
Patch Set 2 : applied changes #
Total comments: 3
Patch Set 3 : #Patch Set 4 : dropped automatically inserted extra line #
Total comments: 6
MessagesTotal messages: 14
Please let me know if I forgot anything or if there is something else to check. Only slightly different thing, visually speaking, is the textarea with filters that is natively grayed out in Edge, but also in FF and Chrome is slightly different. I gues the native OS/Browser UI style there would be more appropriate for users so I don't think that's really an issue. Thanks for double checking (if needed I can attach screenshots too).
Any improvement helps and in the end we'd need to involve QA in it to check that everything works as expected. What they'd do is check the options page against what's defined in the spec so if you want you can do some smoke testing based on the spec to catch obvious issues now already. For the scope of this review, I got a few points though: 1. Please check whether hidden UIs also work (see GET parameters in README to get those to show up). 2. The changes in desktop-options.js don't seem to be necessary for compatibility with Edge. I still think they're valuable though. Usually, we split unrelated changes off into a separate commit so that, in case we need to back out a change, such changes will remain in the code. We can leave them in there this time though if that's easier for you. 3. I'd recommend using the same review title as for the commit you intend to make. Thereby you'll get feedback on the commit message during the review already. In this case, what's missing is the "Issue 123 - " prefix that we use for all of our commits to provide a way to get more context on the change. https://codereview.adblockplus.org/29673576/diff/29673577/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29673576/diff/29673577/desktop-options.js#... desktop-options.js:536: element = element.closest(`[data-${dataName}]`); Nice, I wasn't even aware of this method. :) https://codereview.adblockplus.org/29673576/diff/29673577/desktop-options.js#... desktop-options.js:537: if (element) Detail: We can save one level of indentation by returning `null` here already if we couldn't find an element. https://codereview.adblockplus.org/29673576/diff/29673577/desktop-options.js#... desktop-options.js:539: if (returnElement) return element; Detail: We tend to avoid single-line if-statements to make it clear that, for instance in this case, we're exiting the function. However, to be fair I couldn't find it mentioned anywhere in our coding style. So we may want to think about extending it since this is the way it's written across all of our coide, as far as I'm aware of. https://codereview.adblockplus.org/29673576/diff/29673577/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29673576/diff/29673577/skin/desktop-option... skin/desktop-options.css:228: margin-left: 1rem; We intentionally avoided using properties such as "left", "right", "margin-left", "margin-right", etc. since those refer to the incorrect sides when the UI is shown for right-to-left languages such as arabic or hebrew. Therefore we've been using properties such as "margin-start" or "margin-end" which are independent of the text direction or, in case there is no such workaround, we write additional rules such as `body[dir="rtl"] .side-controls button` to explicitly state the behavior for such languages. https://codereview.adblockplus.org/29673576/diff/29673577/skin/desktop-option... skin/desktop-options.css:618: border-color: transparent; Why did you remove the second value for the border color? https://codereview.adblockplus.org/29673576/diff/29673577/skin/desktop-option... skin/desktop-options.css:628: border-color: #CDCDCD transparent #CDCDCD #CDCDCD; This is the same issue as in the comment regarding using "margin-left" but here it's a bit less obvious since the second value always refers to the right side and the fourth value always refers to the left side.
just replying to Thomas points https://codereview.adblockplus.org/29673576/diff/29673577/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29673576/diff/29673577/desktop-options.js#... desktop-options.js:539: if (returnElement) return element; On 2018/01/18 16:20:46, Thomas Greiner wrote: > Detail: We tend to avoid single-line if-statements to make it clear that, for > instance in this case, we're exiting the function. > > However, to be fair I couldn't find it mentioned anywhere in our coding style. > So we may want to think about extending it since this is the way it's written > across all of our coide, as far as I'm aware of. to be honest, there's no way I am going to remember all the rules so I have a hook in VS Code that formats everything accordingly to the eyeo code style / linting rules, as documented. Do you know if there's a way to enforce this rule? Apparently ESLint does not provide an option for this but I'd love to never care about coding style and also, and of course, I want to preserve consistency. If there's no way to enforce it, I might try to update the documentation specifying that inline if statements are better off our code base. Please let me know how to improve here, thanks. https://codereview.adblockplus.org/29673576/diff/29673577/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29673576/diff/29673577/skin/desktop-option... skin/desktop-options.css:628: border-color: #CDCDCD transparent #CDCDCD #CDCDCD; On 2018/01/18 16:20:46, Thomas Greiner wrote: > This is the same issue as in the comment regarding using "margin-left" but here > it's a bit less obvious since the second value always refers to the right side > and the fourth value always refers to the left side. I was sure there were reasons for using those (unusual accordingly to Microsoft which indeed does not support them) -prefixed-margin-start/end selectors but I didn't know beside text even the UI was changing direction ... which is pretty awesome, and something I need to keep in mind when testing (adding the dir="rtl" part to the body) I'll change all the things to reflect the right direction and double check these are shown properly. Apologies I should've asked right away about -prefix-margin-xxx benefits
https://codereview.adblockplus.org/29673576/diff/29673577/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29673576/diff/29673577/desktop-options.js#... desktop-options.js:539: if (returnElement) return element; On 2018/01/19 11:16:48, a.giammarchi wrote: > to be honest, there's no way I am going to remember all the rules so I have a > hook in VS Code that formats everything accordingly to the eyeo code style / > linting rules, as documented. > > Do you know if there's a way to enforce this rule? Apparently ESLint does not > provide an option for this but I'd love to never care about coding style and > also, and of course, I want to preserve consistency. > > If there's no way to enforce it, I might try to update the documentation > specifying that inline if statements are better off our code base. > > Please let me know how to improve here, thanks. Our coding style wasn't written with compatibility with linters in mind so some rules may not be enforceable in an automatic fashion. For instance, we require usage of hexadecimal notation of CSS colors where possible since that depends on what colors we want to define and what platforms we support. We could investigate whether linters are willing to implement this in which case we could provide this contribution to them. Otherwise we could either modify our coding style or are left with keep doing it manually in which case we simply have to accept that we'll never be able to catch all of the violations. https://codereview.adblockplus.org/29673576/diff/29673577/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29673576/diff/29673577/skin/desktop-option... skin/desktop-options.css:628: border-color: #CDCDCD transparent #CDCDCD #CDCDCD; On 2018/01/19 11:16:48, a.giammarchi wrote: > Apologies I should've asked right away about -prefix-margin-xxx benefits No need to apologize because there's also no explanation for it in the code. We could add inline comments to the spots where we use them to clarify why we need to resort to using non-standard properties, if you think that would've helped.
answering https://codereview.adblockplus.org/29673576/diff/29673577/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29673576/diff/29673577/desktop-options.js#... desktop-options.js:539: if (returnElement) return element; On 2018/01/19 13:55:16, Thomas Greiner wrote: > On 2018/01/19 11:16:48, a.giammarchi wrote: > > to be honest, there's no way I am going to remember all the rules so I have a > > hook in VS Code that formats everything accordingly to the eyeo code style / > > linting rules, as documented. > > > > Do you know if there's a way to enforce this rule? Apparently ESLint does not > > provide an option for this but I'd love to never care about coding style and > > also, and of course, I want to preserve consistency. > > > > If there's no way to enforce it, I might try to update the documentation > > specifying that inline if statements are better off our code base. > > > > Please let me know how to improve here, thanks. > > Our coding style wasn't written with compatibility with linters in mind so some > rules may not be enforceable in an automatic fashion. For instance, we require > usage of hexadecimal notation of CSS colors where possible since that depends on > what colors we want to define and what platforms we support. > > We could investigate whether linters are willing to implement this in which case > we could provide this contribution to them. Otherwise we could either modify our > coding style or are left with keep doing it manually in which case we simply > have to accept that we'll never be able to catch all of the violations. it's good you mentioned that ... 'cause I need a CSS linter too now :-) However, since coding styles are always very subjective, and in my experience in most cases a very pointless waste of everybody time when discussed, I'd love instead to automate all rules and keep consistency. Accordingly, I'll rather improve my automatic-fixing little snippet and maybe share it with others at some point so that consistency and code style will be a no-brainer for everyone, new comer or old. Will keep this `return` rul in mind until I've done that. https://codereview.adblockplus.org/29673576/diff/29673577/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29673576/diff/29673577/skin/desktop-option... skin/desktop-options.css:628: border-color: #CDCDCD transparent #CDCDCD #CDCDCD; On 2018/01/19 13:55:16, Thomas Greiner wrote: > On 2018/01/19 11:16:48, a.giammarchi wrote: > > Apologies I should've asked right away about -prefix-margin-xxx benefits > > No need to apologize because there's also no explanation for it in the code. We > could add inline comments to the spots where we use them to clarify why we need > to resort to using non-standard properties, if you think that would've helped. Yes, it would've helped indeed. I am adding here and there few comments too when I think is appropriate, I would definitively add comments when using prefixed stuff. Problem is, now that we want to support Edge too same Chrome/Firefox way, all prefixed stuff should probably be refactored. I'll have a look in here and see if I can suggest changes in other related CSS too, if needed (or urgent).
Updated as requested. I have left `el.closest(...)` as it is more simple due my current incompetency with Mercurial. THanks for your understanding. https://codereview.adblockplus.org/29673576/diff/29673577/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29673576/diff/29673577/desktop-options.js#... desktop-options.js:537: if (element) On 2018/01/18 16:20:46, Thomas Greiner wrote: > Detail: We can save one level of indentation by returning `null` here already if > we couldn't find an element. Done. https://codereview.adblockplus.org/29673576/diff/29673577/desktop-options.js#... desktop-options.js:539: if (returnElement) return element; On 2018/01/18 16:20:46, Thomas Greiner wrote: > Detail: We tend to avoid single-line if-statements to make it clear that, for > instance in this case, we're exiting the function. > > However, to be fair I couldn't find it mentioned anywhere in our coding style. > So we may want to think about extending it since this is the way it's written > across all of our coide, as far as I'm aware of. Done. https://codereview.adblockplus.org/29673576/diff/29673577/desktop-options.js#... desktop-options.js:539: if (returnElement) return element; On 2018/01/18 16:20:46, Thomas Greiner wrote: > Detail: We tend to avoid single-line if-statements to make it clear that, for > instance in this case, we're exiting the function. > > However, to be fair I couldn't find it mentioned anywhere in our coding style. > So we may want to think about extending it since this is the way it's written > across all of our coide, as far as I'm aware of. Done. https://codereview.adblockplus.org/29673576/diff/29673577/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29673576/diff/29673577/skin/desktop-option... skin/desktop-options.css:228: margin-left: 1rem; On 2018/01/18 16:20:46, Thomas Greiner wrote: > We intentionally avoided using properties such as "left", "right", > "margin-left", "margin-right", etc. since those refer to the incorrect sides > when the UI is shown for right-to-left languages such as arabic or hebrew. > > Therefore we've been using properties such as "margin-start" or "margin-end" > which are independent of the text direction or, in case there is no such > workaround, we write additional rules such as `body[dir="rtl"] .side-controls > button` to explicitly state the behavior for such languages. Done.
Just a typo and comments on seemingly unintentional changes. https://codereview.adblockplus.org/29673576/diff/29673577/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29673576/diff/29673577/desktop-options.js#... desktop-options.js:539: if (returnElement) return element; On 2018/01/19 14:16:28, a.giammarchi wrote: > However, since coding styles are always very subjective, and in my experience in > most cases a very pointless waste of everybody time when discussed, I'd love > instead to automate all rules and keep consistency. That's a very noble approach. I'd be curious to hear what other devs in the company think about that. > Accordingly, I'll rather improve my automatic-fixing little snippet and maybe > share it with others at some point so that consistency and code style will be a > no-brainer for everyone, new comer or old. I'm not too convinced of auto-fixing code personally but detecting all possible violations would be great. Anything we can do to make development more attractive to both internal and external people sounds like a good time investment. https://codereview.adblockplus.org/29673576/diff/29673577/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29673576/diff/29673577/skin/desktop-option... skin/desktop-options.css:628: border-color: #CDCDCD transparent #CDCDCD #CDCDCD; On 2018/01/19 14:16:28, a.giammarchi wrote: > I'll have a look in here and see if I can suggest changes in other related CSS > too, if needed (or urgent). That's fine with me. https://codereview.adblockplus.org/29673576/diff/29676705/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29673576/diff/29676705/skin/desktop-option... skin/desktop-options.css:253: html:not([dir="rtl"]) .side-controls.wrap buttonn Typo: Replace "buttonn" with "button" https://codereview.adblockplus.org/29673576/diff/29676705/skin/desktop-option... skin/desktop-options.css:571: flex-shrink: 0; Detail: Interesting that you added some trailing whitespace here because I assumed your setup prevents you from doing so. https://codereview.adblockplus.org/29673576/diff/29676705/skin/desktop-option... skin/desktop-options.css:693: Detail: I assume those three changes were made by your linter setup so I'd suggest checking your configuration for issues.
fixed all the things! It'd be awesome if we could move this forward, thanks.
LGTM
Message was sent while issue was closed.
Sorry folks this went out of my radar as I wasn't in the CC. https://codereview.adblockplus.org/29673576/diff/29689576/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29673576/diff/29689576/desktop-options.js#... desktop-options.js:536: element = element.closest(`[data-${dataName}]`); Detail: We are usually careful when it comes to experimental technologies. Make sense to find out which Edge versions are we supporting. This feature is only supported in edge 15 and up. Ollie, can you please let us know which versions of Edge are we supporting ? Probably make sense to update that info in as well: https://adblockplus.org/en/requirements https://codereview.adblockplus.org/29673576/diff/29689576/desktop-options.js#... desktop-options.js:1084: if (navigator.doNotTrack != 1) Edge uses window.doNotTrack: https://developer.mozilla.org/en-US/docs/Web/API/Navigator/doNotTrack#Browser...
Message was sent while issue was closed.
https://codereview.adblockplus.org/29673576/diff/29689576/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29673576/diff/29689576/desktop-options.js#... desktop-options.js:536: element = element.closest(`[data-${dataName}]`); On 2018/02/08 14:59:09, saroyanm wrote: > Detail: We are usually careful when it comes to experimental technologies. > > Make sense to find out which Edge versions are we supporting. > This feature is only supported in edge 15 and up. > > Ollie, can you please let us know which versions of Edge are we supporting ? > > Probably make sense to update that info in as well: > https://adblockplus.org/en/requirements FWIW closest is not really experimental, it has been around for years, but in our edge metadata we target 40.15063.0.0 which is >= 15 so I hope there's really nothing to worry about.
Message was sent while issue was closed.
https://codereview.adblockplus.org/29673576/diff/29689576/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29673576/diff/29689576/desktop-options.js#... desktop-options.js:536: element = element.closest(`[data-${dataName}]`); On 2018/02/08 15:07:21, a.giammarchi wrote: > On 2018/02/08 14:59:09, saroyanm wrote: > > Detail: We are usually careful when it comes to experimental technologies. > > > > Make sense to find out which Edge versions are we supporting. > > This feature is only supported in edge 15 and up. > > > > Ollie, can you please let us know which versions of Edge are we supporting ? > > > > Probably make sense to update that info in as well: > > https://adblockplus.org/en/requirements > > FWIW closest is not really experimental, it has been around for years, but in According to MDN it is -> https://developer.mozilla.org/en-US/docs/Web/API/Element/closest But might be their page needs an update. > our edge metadata we target 40.15063.0.0 which is >= 15 so I hope there's really > nothing to worry about. Noted. Anyway I've created a ticket -> https://gitlab.com/eyeo/web.adblockplus.org/issues/17
Message was sent while issue was closed.
https://codereview.adblockplus.org/29673576/diff/29689576/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29673576/diff/29689576/desktop-options.js#... desktop-options.js:536: element = element.closest(`[data-${dataName}]`); On 2018/02/08 15:20:18, saroyanm wrote: > On 2018/02/08 15:07:21, a.giammarchi wrote: > > On 2018/02/08 14:59:09, saroyanm wrote: > > > Detail: We are usually careful when it comes to experimental technologies. > > > > > > Make sense to find out which Edge versions are we supporting. > > > This feature is only supported in edge 15 and up. > > > > > > Ollie, can you please let us know which versions of Edge are we supporting ? > > > > > > Probably make sense to update that info in as well: > > > https://adblockplus.org/en/requirements > > > > FWIW closest is not really experimental, it has been around for years, but in > > According to MDN it is don't trust everything you read in MDN ... it's updated by devs that very often forget to update. closest is in the living standard since years now, it's been polyfilled 3+ years ago -> https://github.com/WebReflection/dom4 and IE15 was last one to implement it. It was a good catch/safety call but also both me and I guess Thomas too checked before shipping it. I'd like to share this little CLI that is configured with our target browsers and tell instantly if we can support this or that. Let's follow up in irc.
Message was sent while issue was closed.
https://codereview.adblockplus.org/29673576/diff/29689576/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29673576/diff/29689576/desktop-options.js#... desktop-options.js:536: element = element.closest(`[data-${dataName}]`); On 2018/02/08 15:27:29, a.giammarchi wrote: > On 2018/02/08 15:20:18, saroyanm wrote: > > On 2018/02/08 15:07:21, a.giammarchi wrote: > > > On 2018/02/08 14:59:09, saroyanm wrote: > > > > Detail: We are usually careful when it comes to experimental technologies. > > > > > > > > Make sense to find out which Edge versions are we supporting. > > > > This feature is only supported in edge 15 and up. > > > > > > > > Ollie, can you please let us know which versions of Edge are we supporting > ? > > > > > > > > Probably make sense to update that info in as well: > > > > https://adblockplus.org/en/requirements > > > > > > FWIW closest is not really experimental, it has been around for years, but > in > > > > According to MDN it is > > don't trust everything you read in MDN ... it's updated by devs that very often > forget to update. > > closest is in the living standard since years now, it's been polyfilled 3+ years > ago -> https://github.com/WebReflection/dom4 and IE15 was last one to implement > it. > > It was a good catch/safety call but also both me and I guess Thomas too checked > before shipping it. > > I'd like to share this little CLI that is configured with our target browsers > and tell instantly if we can support this or that. Let's follow up in irc. Thanks for the links and info. Acknowledged. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
