Issue 29730644: Issue 6514 - IOToggle Custom Element (Closed)
|
Created:
March 22, 2018, 6:18 p.m. by a.giammarchi Modified:
May 2, 2018, 2:09 p.m. Visibility:
Public. |
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 59
Patch Set 4 : applied changes #
Total comments: 4
Patch Set 5 : applied latest required changes #
MessagesTotal messages: 14
This is a testable, stand-alone, Custom Element that looks already like every button role="checkbox" we have, except it does not need any third-parts code to work (the idea is to replace desktop-options.js buttons and related logic with this). It is based on most recent UI style provided by bubble UI, and introduces CSS transitions for a slightly more playful experience. It provides automatic hooks for `action`, `checked`, and `disabled`, and it triggers `change` events like a checkbox would do. It takes care of all ARIA related states, and it's been tested with a11y tools through the smoke test page. This component will be needed for the new language list, possibly after all toggles in our pages have been replaced (as separated task). This will allow us to both simplify the layout and shrink logic of the monolith desktop-options.js file.
Thomas, Manvel, please have a look at this very first component that is still in codereview, thanks.
Forgot to add the live demo: https://webreflection.github.io/eyeo/custom-elements/smoke/io-toggle.html
This overall looks good, just couple of clarification comments and suggestions. https://codereview.adblockplus.org/29730644/diff/29733654/css/io-toggle.scss File css/io-toggle.scss (right): https://codereview.adblockplus.org/29730644/diff/29733654/css/io-toggle.scss#... css/io-toggle.scss:2: --width: 30px; While we are using SASS, wouldn't that make more sense to use SASS variables instead of the CSS variables ? SCSS variables are still powerful I think because we can still define them in the separate file and reuse them in modules. If you folks agree, maybe we can switch to using SASS variables instead ? https://codereview.adblockplus.org/29730644/diff/29733654/css/io-toggle.scss#... css/io-toggle.scss:10: background-color: #9b9b9b; Having colors set separately will make it easy to keep it in sync with the specifications I think -> https://gitlab.com/eyeo/specs/spec/blob/master/spec/abp/options-page-style-gu... We might want to update the colors and even better separate components in the specifications and apply new colors defined here if they are already agreed with Jeen. https://codereview.adblockplus.org/29730644/diff/29733654/tests/io-toggle.html File tests/io-toggle.html (right): https://codereview.adblockplus.org/29730644/diff/29733654/tests/io-toggle.htm... tests/io-toggle.html:6: <link rel="stylesheet" href="../skin/common.css"> Seems like The toggle buttons are not dependent on most of the styles and scripts here. I suggest to remove all the scripts and styles that have nothing to do with current component, in order not to create a false assumption. https://codereview.adblockplus.org/29730644/diff/29733654/tests/io-toggle.htm... tests/io-toggle.html:25: <fieldset><io-toggle onchange="console.log(event)"></io-toggle></fieldset> Suggestion: I suggest not include console.logs in current test in order to prevent this being copy pasted to the production, by accident.
Sorry for the long wait. Looks like that review was created while I was on holiday. https://codereview.adblockplus.org/29730644/diff/29733654/css/desktop-options... File css/desktop-options.scss (right): https://codereview.adblockplus.org/29730644/diff/29733654/css/desktop-options... css/desktop-options.scss:18: @import "io-toggle.scss"; You're not using this component in the desktop options page yet so I'd recommend making this change together with the one where we do that. https://codereview.adblockplus.org/29730644/diff/29733654/css/io-toggle.scss File css/io-toggle.scss (right): https://codereview.adblockplus.org/29730644/diff/29733654/css/io-toggle.scss#... css/io-toggle.scss:1: io-toggle { Coding style: "Opening braces always go on their own line." https://codereview.adblockplus.org/29730644/diff/29733654/css/io-toggle.scss#... css/io-toggle.scss:11: transition: background .2s ease-out; Coding style: "Don't omit the optional leading 0 for decimal numbers." https://codereview.adblockplus.org/29730644/diff/29733654/css/io-toggle.scss#... css/io-toggle.scss:11: transition: background .2s ease-out; Technically, we only expect "background-color" to change so we could restrict "transition" and "will-change" to refer to "background-color". https://codereview.adblockplus.org/29730644/diff/29733654/css/io-toggle.scss#... css/io-toggle.scss:17: background-color: #92d3ea; How did you choose the colors? At least this one isn't listed at https://gitlab.com/eyeo/specs/spec/blob/master/spec/abp/options-page-style-gu.... https://codereview.adblockplus.org/29730644/diff/29733654/css/io-toggle.scss#... css/io-toggle.scss:69: /* in case we will need to switch active/disabled direction I was also wondering about that and it appears that the direction should indeed be switched for RTL locales. Although I didn't find any authoritative sources for that, only those references: - https://bugzilla.mozilla.org/show_bug.cgi?id=1414985 - https://docs.oracle.com/cd/E87544_01/pt856pbr1/eng/pt/tgbl/task_WorkingWithBi... - https://rtl-css.net/tutorial/quick-tip-rtling-css3-box-shadow-property https://codereview.adblockplus.org/29730644/diff/29733654/js/io-toggle.js File js/io-toggle.js (right): https://codereview.adblockplus.org/29730644/diff/29733654/js/io-toggle.js#new... js/io-toggle.js:1: /* globals module, require */ I noticed that none of the new files in this review contain our license header so let's add that. https://codereview.adblockplus.org/29730644/diff/29733654/js/io-toggle.js#new... js/io-toggle.js:1: /* globals module, require */ Detail: You don't use `module` in this module. https://codereview.adblockplus.org/29730644/diff/29733654/js/io-toggle.js#new... js/io-toggle.js:40: this.firstElementChild.disabled = this._disabled; Why don't you call `this.render()` here? https://codereview.adblockplus.org/29730644/diff/29733654/js/io-toggle.js#new... js/io-toggle.js:74: function asBoolean(value) Suggestion: "asBoolean" is quite close to "isBoolean" so it may cause issues. Therefore what about using a more distinct name such as "toBoolean"? https://codereview.adblockplus.org/29730644/diff/29733654/js/io-toggle.js#new... js/io-toggle.js:76: return typeof value === "string" ? JSON.parse(value) : !!value; It's a bit unexpected that this function accepts everything except for strings which contain invalid JSON. So is it intentional that you throw an error here when a string with invalid JSON is passed? https://codereview.adblockplus.org/29730644/diff/29733654/js/io-toggle.js#new... js/io-toggle.js:79: function booleanAttribute(name, value) Detail: This function looks useful also for other components so what about moving it to a shared location? https://codereview.adblockplus.org/29730644/diff/29733654/tests/io-toggle.html File tests/io-toggle.html (right): https://codereview.adblockplus.org/29730644/diff/29733654/tests/io-toggle.htm... tests/io-toggle.html:9: <style> I know that it's just a test but I think it'd be best to stick to existing conventions such as separating styles from the HTML. That way we can also lint any test-related resources such as these styles.
Thanks a LOT for finally reviewing this! I have hopefully clarified few points, answered others, and will take actions to other points too. Please let me know what you think about my answers and, where appropriate, please give me hints how to proceed (i.e. colors somewhere else, I'd go for it, but I wouldn't know where/how) Thanks again. https://codereview.adblockplus.org/29730644/diff/29733654/css/desktop-options... File css/desktop-options.scss (right): https://codereview.adblockplus.org/29730644/diff/29733654/css/desktop-options... css/desktop-options.scss:18: @import "io-toggle.scss"; On 2018/04/25 17:29:13, Thomas Greiner wrote: > You're not using this component in the desktop options page yet so I'd recommend > making this change together with the one where we do that. Fair point, but that is why I was using `require` to include CSS too, if you remember. I otherwise have no easy way to test components within the desktop style. I'll think about a solution, if you have any suggestion, that'd be welcome. https://codereview.adblockplus.org/29730644/diff/29733654/css/io-toggle.scss File css/io-toggle.scss (right): https://codereview.adblockplus.org/29730644/diff/29733654/css/io-toggle.scss#... css/io-toggle.scss:1: io-toggle { On 2018/04/25 17:29:14, Thomas Greiner wrote: > Coding style: "Opening braces always go on their own line." have we published this eyeo style thing somewhere so I can fix this stuff automatically? https://codereview.adblockplus.org/29730644/diff/29733654/css/io-toggle.scss#... css/io-toggle.scss:2: --width: 30px; On 2018/04/25 17:05:01, saroyanm wrote: > While we are using SASS, wouldn't that make more sense to use SASS variables > instead of the CSS variables ? > SCSS variables are still powerful I think because we can still define them in > the separate file and reuse them in modules. > > If you folks agree, maybe we can switch to using SASS variables instead ? I think the whole point of dropping Edge 14 was to be able to use native CSS variables. These variables can be manipulated, if needed, by JS, while SASS variables cannot. However, I agree if/when we have the need to share immutable variables across components we can prefer SASS variables over CSS variables. Forcing all variables to be SASS variables seems unnecessary seppuku for the future of the UI. https://codereview.adblockplus.org/29730644/diff/29733654/css/io-toggle.scss#... css/io-toggle.scss:10: background-color: #9b9b9b; On 2018/04/25 17:05:01, saroyanm wrote: > Having colors set separately will make it easy to keep it in sync with the > specifications I think -> > https://gitlab.com/eyeo/specs/spec/blob/master/spec/abp/options-page-style-gu... > > > We might want to update the colors and even better separate components in the > specifications and apply new colors defined here if they are already agreed with > Jeen. Yeah. Colors might be one of those immutable shared variables, but we have to agree on where/how to use them across all UI. https://codereview.adblockplus.org/29730644/diff/29733654/css/io-toggle.scss#... css/io-toggle.scss:11: transition: background .2s ease-out; On 2018/04/25 17:29:13, Thomas Greiner wrote: > Technically, we only expect "background-color" to change so we could restrict > "transition" and "will-change" to refer to "background-color". AFAIK that doesn't work. will change wants background, same as margin, or padding. https://codereview.adblockplus.org/29730644/diff/29733654/css/io-toggle.scss#... css/io-toggle.scss:17: background-color: #92d3ea; On 2018/04/25 17:29:13, Thomas Greiner wrote: > How did you choose the colors? At least this one isn't listed at > https://gitlab.com/eyeo/specs/spec/blob/master/spec/abp/options-page-style-gu.... I have downloaded specs images and picked color through GIMP color picker. https://codereview.adblockplus.org/29730644/diff/29733654/css/io-toggle.scss#... css/io-toggle.scss:69: /* in case we will need to switch active/disabled direction On 2018/04/25 17:29:13, Thomas Greiner wrote: > I was also wondering about that and it appears that the direction should indeed > be switched for RTL locales. Although I didn't find any authoritative sources > for that, only those references: > > - https://bugzilla.mozilla.org/show_bug.cgi?id=1414985 > - > https://docs.oracle.com/cd/E87544_01/pt856pbr1/eng/pt/tgbl/task_WorkingWithBi... > - https://rtl-css.net/tutorial/quick-tip-rtling-css3-box-shadow-property it took me a little while to figure this out, so I've preferred leaving it as comment to eventually make active whenever we'll decide to do this. https://codereview.adblockplus.org/29730644/diff/29733654/js/io-toggle.js File js/io-toggle.js (right): https://codereview.adblockplus.org/29730644/diff/29733654/js/io-toggle.js#new... js/io-toggle.js:1: /* globals module, require */ On 2018/04/25 17:29:14, Thomas Greiner wrote: > I noticed that none of the new files in this review contain our license header > so let's add that. Will do. https://codereview.adblockplus.org/29730644/diff/29733654/js/io-toggle.js#new... js/io-toggle.js:1: /* globals module, require */ On 2018/04/25 17:29:14, Thomas Greiner wrote: > Detail: You don't use `module` in this module. To be honest, I feel like we should have module and require as always allowed in our eslint configuration since we decided to use CommonJS for the time being. https://codereview.adblockplus.org/29730644/diff/29733654/js/io-toggle.js#new... js/io-toggle.js:40: this.firstElementChild.disabled = this._disabled; On 2018/04/25 17:29:14, Thomas Greiner wrote: > Why don't you call `this.render()` here? it's not needed, the disabled is the native DOM accessor and the render wouldn't care less. Just as extra info, hyperHTML does not create new DOM per each render, it simply updates what it's responsible for through nodes that are always the same. You can references these nodes even after rendering them, it does not lock you in. https://codereview.adblockplus.org/29730644/diff/29733654/js/io-toggle.js#new... js/io-toggle.js:74: function asBoolean(value) On 2018/04/25 17:29:14, Thomas Greiner wrote: > Suggestion: "asBoolean" is quite close to "isBoolean" so it may cause issues. > Therefore what about using a more distinct name such as "toBoolean"? will do https://codereview.adblockplus.org/29730644/diff/29733654/js/io-toggle.js#new... js/io-toggle.js:76: return typeof value === "string" ? JSON.parse(value) : !!value; On 2018/04/25 17:29:14, Thomas Greiner wrote: > It's a bit unexpected that this function accepts everything except for strings > which contain invalid JSON. So is it intentional that you throw an error here > when a string with invalid JSON is passed? only strings can be JSON, and in this case we are talking about string `true` and string `false` as attributes value. also string 1 and string 0 will be OK. Any other value should never be allowed so I would expect an error. https://codereview.adblockplus.org/29730644/diff/29733654/js/io-toggle.js#new... js/io-toggle.js:79: function booleanAttribute(name, value) On 2018/04/25 17:29:14, Thomas Greiner wrote: > Detail: This function looks useful also for other components so what about > moving it to a shared location? Usually the way I go is: as soon as more than a component needs it, I'll create that file. Right now I don't remember if any other components needs something like this, but if this could be the starting point to create shared DOM utils then let's do it. Where do you think such utility should go? https://codereview.adblockplus.org/29730644/diff/29733654/tests/io-toggle.html File tests/io-toggle.html (right): https://codereview.adblockplus.org/29730644/diff/29733654/tests/io-toggle.htm... tests/io-toggle.html:6: <link rel="stylesheet" href="../skin/common.css"> On 2018/04/25 17:05:01, saroyanm wrote: > Seems like The toggle buttons are not dependent on most of the styles and > scripts here. > > I suggest to remove all the scripts and styles that have nothing to do with > current component, in order not to create a false assumption. it is mandatory to test components UI together with common UI rules to be sure there are no interferences, IMO. This is also some sort of testing template for all occasions. https://codereview.adblockplus.org/29730644/diff/29733654/tests/io-toggle.htm... tests/io-toggle.html:9: <style> On 2018/04/25 17:29:14, Thomas Greiner wrote: > I know that it's just a test but I think it'd be best to stick to existing > conventions such as separating styles from the HTML. That way we can also lint > any test-related resources such as these styles. If we make writing tests hard, we'll most likely will to skip writing them. I think we should be as free as possible for tests, of course inside the limit of decency and readability. A file for a single fieldset unnecessary for anything else seems overkill to me. https://codereview.adblockplus.org/29730644/diff/29733654/tests/io-toggle.htm... tests/io-toggle.html:25: <fieldset><io-toggle onchange="console.log(event)"></io-toggle></fieldset> On 2018/04/25 17:05:01, saroyanm wrote: > Suggestion: I suggest not include console.logs in current test in order to > prevent this being copy pasted to the production, by accident. fair enough, but please consider my answer to Thomas too. We should probably be easy-going with tests, IMO. Code reviews would easily spot a copy and paste mistake.
It'd be great if we could find a balance between making it easy to write tests and keeping our code reviewable. Because both parts are worth striving for IMHO. https://codereview.adblockplus.org/29730644/diff/29733654/css/desktop-options... File css/desktop-options.scss (right): https://codereview.adblockplus.org/29730644/diff/29733654/css/desktop-options... css/desktop-options.scss:18: @import "io-toggle.scss"; On 2018/04/26 10:44:50, a.giammarchi wrote: > On 2018/04/25 17:29:13, Thomas Greiner wrote: > > You're not using this component in the desktop options page yet so I'd > recommend > > making this change together with the one where we do that. > > Fair point, but that is why I was using `require` to include CSS too, if you > remember. > I otherwise have no easy way to test components within the desktop style. > > I'll think about a solution, if you have any suggestion, that'd be welcome. I see. What about generating CSS files for each component before running the tests? That way we wouldn't need to include unrelated styles such as the ones in desktop-options.css in tests/io-toggle.html. https://codereview.adblockplus.org/29730644/diff/29733654/css/io-toggle.scss File css/io-toggle.scss (right): https://codereview.adblockplus.org/29730644/diff/29733654/css/io-toggle.scss#... css/io-toggle.scss:1: io-toggle { On 2018/04/26 10:44:51, a.giammarchi wrote: > On 2018/04/25 17:29:14, Thomas Greiner wrote: > > Coding style: "Opening braces always go on their own line." > > have we published this eyeo style thing somewhere so I can fix this stuff > automatically? Yep, two weeks ago we've published the stylelint-config-eyeo package on npm so we are finally able to use that to lint our CSS code. https://codereview.adblockplus.org/29730644/diff/29733654/css/io-toggle.scss#... css/io-toggle.scss:2: --width: 30px; I'd argue that we want to rely on web standards as much as possible to avoid any unnecessary "magic" in between the developer and the user. That being said, SASS has the benefit of allowing us to generate CSS files at build time which is useful for modularization. Furthermore, if SASS supports a feature we'd expect to become part of the standard, we could also use it as a polyfill. https://codereview.adblockplus.org/29730644/diff/29733654/css/io-toggle.scss#... css/io-toggle.scss:11: transition: background .2s ease-out; On 2018/04/26 10:44:51, a.giammarchi wrote: > AFAIK that doesn't work. will change wants background, same as margin, or > padding. Interesting. I wasn't aware of that. No problem with keeping it as is then. https://codereview.adblockplus.org/29730644/diff/29733654/css/io-toggle.scss#... css/io-toggle.scss:17: background-color: #92d3ea; On 2018/04/26 10:44:51, a.giammarchi wrote: > I have downloaded specs images and picked color through GIMP color picker. Since the part of the spec I referred to only applies to the options page, I'd be fine with the colors you chose but suggest to quickly check back with Jeen. https://codereview.adblockplus.org/29730644/diff/29733654/css/io-toggle.scss#... css/io-toggle.scss:69: /* in case we will need to switch active/disabled direction For some reason I thought you were talking about the direction of `box-shadow` here because I initially intended to comment on that above. But looking at it again you're referring to the toggle direction in general. https://ux.stackexchange.com/questions/113633/should-a-toggle-switchs-label-b... is the best discussion I found about it which suggests to indeed switch the toggle direction. In any case it shouldn't harm to double-check with Jeen and update the spec to reflect such decisions. Besides that, if this weren't the case, our coding style states: "Don't comment code out, delete it." https://codereview.adblockplus.org/29730644/diff/29733654/js/io-toggle.js File js/io-toggle.js (right): https://codereview.adblockplus.org/29730644/diff/29733654/js/io-toggle.js#new... js/io-toggle.js:1: /* globals module, require */ On 2018/04/26 10:44:52, a.giammarchi wrote: > To be honest, I feel like we should have module and require as always allowed in > our eslint configuration since we decided to use CommonJS for the time being. Wouldn't .eslintrc.json be a more suitable place to put that then? https://codereview.adblockplus.org/29730644/diff/29733654/js/io-toggle.js#new... js/io-toggle.js:40: this.firstElementChild.disabled = this._disabled; On 2018/04/26 10:44:52, a.giammarchi wrote: > On 2018/04/25 17:29:14, Thomas Greiner wrote: > > Why don't you call `this.render()` here? > > it's not needed, the disabled is the native DOM accessor and the render wouldn't > care less. > > Just as extra info, hyperHTML does not create new DOM per each render, it simply > updates what it's responsible for through nodes that are always the same. > > You can references these nodes even after rendering them, it does not lock you > in. I mean instead of setting `this.firstElementChild.disabled = this._disabled` setting the button's disabled attribute in `render()`. That way we wouldn't need to worry about what the component's first child element is. Also, all of the rendering logic would be kept in a single location. https://codereview.adblockplus.org/29730644/diff/29733654/js/io-toggle.js#new... js/io-toggle.js:76: return typeof value === "string" ? JSON.parse(value) : !!value; On 2018/04/26 10:44:52, a.giammarchi wrote: > On 2018/04/25 17:29:14, Thomas Greiner wrote: > > It's a bit unexpected that this function accepts everything except for strings > > which contain invalid JSON. So is it intentional that you throw an error here > > when a string with invalid JSON is passed? > > only strings can be JSON, and in this case we are talking about string `true` > and string `false` as attributes value. > > also string 1 and string 0 will be OK. Any other value should never be allowed > so I would expect an error. I can follow your line or argument right until the last part because why would `"123"` throw if `123` doesn't? So instead, what about the following so that we treat values the same way? if (typeof value === "string") { try { value = JSON.parse(value); } catch (ex) { // Ignore invalid JSON to continue using value as string } } return !!value; https://codereview.adblockplus.org/29730644/diff/29733654/js/io-toggle.js#new... js/io-toggle.js:79: function booleanAttribute(name, value) On 2018/04/26 10:44:51, a.giammarchi wrote: > Usually the way I go is: as soon as more than a component needs it, I'll create > that file. > > Right now I don't remember if any other components needs something like this, > but if this could be the starting point to create shared DOM utils then let's do > it. That's a fair point. And it should be fine as long as we don't duplicate functions across multiple files because we forgot that we already have them somewhere. > Where do you think such utility should go? Not sure, maybe js/io-element.js?! At least it could be useful for any custom element we'll introduce. https://codereview.adblockplus.org/29730644/diff/29733654/tests/io-toggle.html File tests/io-toggle.html (right): https://codereview.adblockplus.org/29730644/diff/29733654/tests/io-toggle.htm... tests/io-toggle.html:6: <link rel="stylesheet" href="../skin/common.css"> On 2018/04/26 10:44:52, a.giammarchi wrote: > On 2018/04/25 17:05:01, saroyanm wrote: > > Seems like The toggle buttons are not dependent on most of the styles and > > scripts here. > > > > I suggest to remove all the scripts and styles that have nothing to do with > > current component, in order not to create a false assumption. > > it is mandatory to test components UI together with common UI rules to be sure > there are no interferences, IMO. This is also some sort of testing template for > all occasions. Acknowledged. https://codereview.adblockplus.org/29730644/diff/29733654/tests/io-toggle.htm... tests/io-toggle.html:9: <style> On 2018/04/26 10:44:52, a.giammarchi wrote: > If we make writing tests hard, we'll most likely will to skip writing them. > > I think we should be as free as possible for tests, of course inside the limit > of decency and readability. While it sounds reasonable at first glance, I'm not sure how much of a hurdle it really is to stay consistent with our coding style or are you suggesting that we should not review tests? > A file for a single fieldset unnecessary for anything else seems overkill to me. I was thinking more along the lines of having a CSS file for tests which would also include the body's background color. However, since having inline CSS is not against any rules, I'm quite relaxed about keeping it inline because I agree with your point that it's better to keep things simple.
working on the rest. https://codereview.adblockplus.org/29730644/diff/29733654/css/io-toggle.scss File css/io-toggle.scss (right): https://codereview.adblockplus.org/29730644/diff/29733654/css/io-toggle.scss#... css/io-toggle.scss:2: --width: 30px; On 2018/04/26 17:00:51, Thomas Greiner wrote: > I'd argue that we want to rely on web standards as much as possible to avoid any > unnecessary "magic" in between the developer and the user. > > That being said, SASS has the benefit of allowing us to generate CSS files at > build time which is useful for modularization. Furthermore, if SASS supports a > feature we'd expect to become part of the standard, we could also use it as a > polyfill. this doesn't address the fact CSS variables are already out, supported, and the standard, and at this point more useful/powerful than SASS variables. We can use CSS variables also in a file a part, configured under `:root { ... }` to share colors. If we are pursuing standards, CSS variables are already available to our UI and are more powerful, 'cause accessible through JS. This is very important since we started seeing UI implementing animations, where these performs always better through CSS rather than JS. Accordingly, if we have to chose between standards and SASS variables *only*, then I'd vote standard, 'cause I don't see any advantage in SASS since we don't need pre processing anything with just CSS. Actually, once we have a way to import CSS per page, if we ever need that, we won't need SASS at all for the kind of style we usually write. https://codereview.adblockplus.org/29730644/diff/29733654/css/io-toggle.scss#... css/io-toggle.scss:17: background-color: #92d3ea; On 2018/04/26 17:00:51, Thomas Greiner wrote: > On 2018/04/26 10:44:51, a.giammarchi wrote: > > I have downloaded specs images and picked color through GIMP color picker. > > Since the part of the spec I referred to only applies to the options page, I'd > be fine with the colors you chose but suggest to quickly check back with Jeen. AFAIK Jeen already approved this component and wants it all over our UI :-) https://codereview.adblockplus.org/29730644/diff/29733654/css/io-toggle.scss#... css/io-toggle.scss:69: /* in case we will need to switch active/disabled direction On 2018/04/26 17:00:52, Thomas Greiner wrote: > For some reason I thought you were talking about the direction of `box-shadow` > here because I initially intended to comment on that above. > > But looking at it again you're referring to the toggle direction in general. > https://ux.stackexchange.com/questions/113633/should-a-toggle-switchs-label-b... > is the best discussion I found about it which suggests to indeed switch the > toggle direction. > In any case it shouldn't harm to double-check with Jeen and update the spec to > reflect such decisions. > > Besides that, if this weren't the case, our coding style states: "Don't comment > code out, delete it." I'll double check with Jeen before eventually removing this bit, thanks. https://codereview.adblockplus.org/29730644/diff/29733654/js/io-toggle.js File js/io-toggle.js (right): https://codereview.adblockplus.org/29730644/diff/29733654/js/io-toggle.js#new... js/io-toggle.js:1: /* globals module, require */ On 2018/04/26 17:00:52, Thomas Greiner wrote: > On 2018/04/26 10:44:52, a.giammarchi wrote: > > To be honest, I feel like we should have module and require as always allowed > in > > our eslint configuration since we decided to use CommonJS for the time being. > > Wouldn't .eslintrc.json be a more suitable place to put that then? absolutely! https://codereview.adblockplus.org/29730644/diff/29733654/js/io-toggle.js#new... js/io-toggle.js:40: this.firstElementChild.disabled = this._disabled; On 2018/04/26 17:00:52, Thomas Greiner wrote: > On 2018/04/26 10:44:52, a.giammarchi wrote: > > On 2018/04/25 17:29:14, Thomas Greiner wrote: > > > Why don't you call `this.render()` here? > > > > it's not needed, the disabled is the native DOM accessor and the render > wouldn't > > care less. > > > > Just as extra info, hyperHTML does not create new DOM per each render, it > simply > > updates what it's responsible for through nodes that are always the same. > > > > You can references these nodes even after rendering them, it does not lock you > > in. > > I mean instead of setting `this.firstElementChild.disabled = this._disabled` > setting the button's disabled attribute in `render()`. That way we wouldn't need > to worry about what the component's first child element is. Also, all of the > rendering logic would be kept in a single location. I'll try that. The issue here is that disabled is an accessor. Setting it as attribute would not produce the accessor effect, will just set such attribute. However, what you says makes sense so let's see how that works. https://codereview.adblockplus.org/29730644/diff/29733654/js/io-toggle.js#new... js/io-toggle.js:76: return typeof value === "string" ? JSON.parse(value) : !!value; On 2018/04/26 17:00:52, Thomas Greiner wrote: > On 2018/04/26 10:44:52, a.giammarchi wrote: > > On 2018/04/25 17:29:14, Thomas Greiner wrote: > > > It's a bit unexpected that this function accepts everything except for > strings > > > which contain invalid JSON. So is it intentional that you throw an error > here > > > when a string with invalid JSON is passed? > > > > only strings can be JSON, and in this case we are talking about string `true` > > and string `false` as attributes value. > > > > also string 1 and string 0 will be OK. Any other value should never be allowed > > so I would expect an error. > > I can follow your line or argument right until the last part because why would > `"123"` throw if `123` doesn't? > > So instead, what about the following so that we treat values the same way? > > if (typeof value === "string") > { > try > { > value = JSON.parse(value); > } > catch (ex) > { > // Ignore invalid JSON to continue using value as string > } > } > > return !!value; will do. https://codereview.adblockplus.org/29730644/diff/29733654/js/io-toggle.js#new... js/io-toggle.js:79: function booleanAttribute(name, value) On 2018/04/26 17:00:52, Thomas Greiner wrote: > On 2018/04/26 10:44:51, a.giammarchi wrote: > > Usually the way I go is: as soon as more than a component needs it, I'll > create > > that file. > > > > Right now I don't remember if any other components needs something like this, > > but if this could be the starting point to create shared DOM utils then let's > do > > it. > > That's a fair point. And it should be fine as long as we don't duplicate > functions across multiple files because we forgot that we already have them > somewhere. > > > Where do you think such utility should go? > > Not sure, maybe js/io-element.js?! At least it could be useful for any custom > element we'll introduce. makes sense. https://codereview.adblockplus.org/29730644/diff/29733654/tests/io-toggle.html File tests/io-toggle.html (right): https://codereview.adblockplus.org/29730644/diff/29733654/tests/io-toggle.htm... tests/io-toggle.html:6: <link rel="stylesheet" href="../skin/common.css"> On 2018/04/25 17:05:01, saroyanm wrote: > Seems like The toggle buttons are not dependent on most of the styles and > scripts here. > > I suggest to remove all the scripts and styles that have nothing to do with > current component, in order not to create a false assumption. I forgot to answer this Manvel point. The `IOElement` class uses `ext.i18n` to simplify the inclusion of i18n text in other components too. It is not possible to test anything without other scripts, and AFAIK all of them are needed to have `ext.i18n` available and working. When we'll finally decide to have modules for these global dependencies it'd be easy to drop every unrelated script. Until we have these global dependencies, those scripts are needed. https://codereview.adblockplus.org/29730644/diff/29733654/tests/io-toggle.htm... tests/io-toggle.html:9: <style> On 2018/04/26 17:00:52, Thomas Greiner wrote: > On 2018/04/26 10:44:52, a.giammarchi wrote: > > If we make writing tests hard, we'll most likely will to skip writing them. > > > > I think we should be as free as possible for tests, of course inside the limit > > of decency and readability. > > While it sounds reasonable at first glance, I'm not sure how much of a hurdle it > really is to stay consistent with our coding style or are you suggesting that we > should not review tests? > > > A file for a single fieldset unnecessary for anything else seems overkill to > me. > > I was thinking more along the lines of having a CSS file for tests which would > also include the body's background color. > > However, since having inline CSS is not against any rules, I'm quite relaxed > about keeping it inline because I agree with your point that it's better to keep > things simple. I am not saying tests should not be reviewed, I am saying tests might contain incredible hacks to be able to work stand alone and if we are not *super* relaxed about tests every component will ship in 3X the time it would need without tests.
I have done some cleanup and I should've addressed all required changes. Please let me know if I've forgotten something, thanks.
https://codereview.adblockplus.org/29730644/diff/29763743/css/io-toggle.scss File css/io-toggle.scss (right): https://codereview.adblockplus.org/29730644/diff/29763743/css/io-toggle.scss#... css/io-toggle.scss:28: background-color: #9b9b9b; There were some complex discussions, regarding SASS.. I don't feel like commenting under previous code as I keep missing old discussions. I didn't understand the arguments in the previous discussion completely, because they are arguable and they "depends". So what about moving Color names into separate file, called _variables.SCSS ? I don't know if here we have common sizes, but I think colors are good start. That way we will avoid having color discussions in the reviews in future and hopefully eventually sizes as well. If there is other native way to do that which doesn't require additional request, it's again fine with me again, but in that case we might want to abandon SCSS completely I assume, as I currently don't see any other SCSS use-case which will make all developers happy, maybe only for importing website defaults, which again make things complex. FWIW: I do like using preprocessors, I would even be in favor of using some of it's "magic". That will make this CSS looks more readable: But that might be personal preference: @import "_variables.less"; io-toggle { &[checked] { background-color: @primary; ... } &[disabled] { ... } } # tl;dr I'm fine using anything that will provide way to share variables across component, pages and ideally modules(website). If we have arguments against it, than I don't know why we have introduced SASS in first place, seems like we don't need it?
Just two small things left. https://codereview.adblockplus.org/29730644/diff/29733654/css/io-toggle.scss File css/io-toggle.scss (right): https://codereview.adblockplus.org/29730644/diff/29733654/css/io-toggle.scss#... css/io-toggle.scss:2: --width: 30px; On 2018/04/27 10:59:04, a.giammarchi wrote: > this doesn't address the fact CSS variables are already out, supported, and the > standard, and at this point more useful/powerful than SASS variables. > > We can use CSS variables also in a file a part, configured under `:root { ... }` > to share colors. > > If we are pursuing standards, CSS variables are already available to our UI and > are more powerful, 'cause accessible through JS. > > This is very important since we started seeing UI implementing animations, where > these performs always better through CSS rather than JS. > > Accordingly, if we have to chose between standards and SASS variables *only*, > then I'd vote standard, 'cause I don't see any advantage in SASS since we don't > need pre processing anything with just CSS. > > Actually, once we have a way to import CSS per page, if we ever need that, we > won't need SASS at all for the kind of style we usually write. That's also what I'm saying that we should always prefer web standards over SASS and I also agree that as soon as we don't need SASS anymore that we should drop it. https://codereview.adblockplus.org/29730644/diff/29733654/css/io-toggle.scss#... css/io-toggle.scss:17: background-color: #92d3ea; On 2018/04/27 10:59:04, a.giammarchi wrote: > On 2018/04/26 17:00:51, Thomas Greiner wrote: > > On 2018/04/26 10:44:51, a.giammarchi wrote: > > > I have downloaded specs images and picked color through GIMP color picker. > > > > Since the part of the spec I referred to only applies to the options page, I'd > > be fine with the colors you chose but suggest to quickly check back with Jeen. > > AFAIK Jeen already approved this component and wants it all over our UI :-) Acknowledged. https://codereview.adblockplus.org/29730644/diff/29733654/css/io-toggle.scss#... css/io-toggle.scss:69: /* in case we will need to switch active/disabled direction On 2018/04/27 10:59:04, a.giammarchi wrote: > On 2018/04/26 17:00:52, Thomas Greiner wrote: > > For some reason I thought you were talking about the direction of `box-shadow` > > here because I initially intended to comment on that above. > > > > But looking at it again you're referring to the toggle direction in general. > > > https://ux.stackexchange.com/questions/113633/should-a-toggle-switchs-label-b... > > is the best discussion I found about it which suggests to indeed switch the > > toggle direction. > > In any case it shouldn't harm to double-check with Jeen and update the spec to > > reflect such decisions. > > > > Besides that, if this weren't the case, our coding style states: "Don't > comment > > code out, delete it." > > I'll double check with Jeen before eventually removing this bit, thanks. Acknowledged. https://codereview.adblockplus.org/29730644/diff/29733654/js/io-toggle.js File js/io-toggle.js (right): https://codereview.adblockplus.org/29730644/diff/29733654/js/io-toggle.js#new... js/io-toggle.js:40: this.firstElementChild.disabled = this._disabled; On 2018/04/27 10:59:04, a.giammarchi wrote: > I'll try that. The issue here is that disabled is an accessor. Setting it as > attribute would not produce the accessor effect, will just set such attribute. > > However, what you says makes sense so let's see how that works. Thanks, looking good! https://codereview.adblockplus.org/29730644/diff/29733654/tests/io-toggle.html File tests/io-toggle.html (right): https://codereview.adblockplus.org/29730644/diff/29733654/tests/io-toggle.htm... tests/io-toggle.html:9: <style> On 2018/04/27 10:59:05, a.giammarchi wrote: > I am not saying tests should not be reviewed, I am saying tests might contain > incredible hacks to be able to work stand alone and if we are not *super* > relaxed about tests every component will ship in 3X the time it would need > without tests. I think we agree on all points and just need to agree on how we want to balance those two. So I'm fine with going forward with this to finish this review and discussing how we want to write tests with Manvel and Winsley separately. https://codereview.adblockplus.org/29730644/diff/29763743/skin/io-toggle.css File skin/io-toggle.css (right): https://codereview.adblockplus.org/29730644/diff/29763743/skin/io-toggle.css#... skin/io-toggle.css:1: io-toggle { I assume this auto-generated file made it into the review because it's not listed in .{git,hg}ignore. https://codereview.adblockplus.org/29730644/diff/29763743/tests/io-toggle.js File tests/io-toggle.js (right): https://codereview.adblockplus.org/29730644/diff/29763743/tests/io-toggle.js#... tests/io-toggle.js:1: /* globals require */ Detail: This should no longer be necessary now.
Addressed last changes asked by Thomas https://codereview.adblockplus.org/29730644/diff/29763743/skin/io-toggle.css File skin/io-toggle.css (right): https://codereview.adblockplus.org/29730644/diff/29763743/skin/io-toggle.css#... skin/io-toggle.css:1: io-toggle { On 2018/05/02 11:53:28, Thomas Greiner wrote: > I assume this auto-generated file made it into the review because it's not > listed in .{git,hg}ignore. oooops, dropped.
LGTM
Moved into Gitlab: https://gitlab.com/eyeo/adblockplus/adblockplusui/merge_requests/12 Closing this codereview. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
