Adding rainhrad@ as well, as a more specific OWNER.
#httpsOnlyModeToggle, #passwordsLeakToggleMove {
Each selector should be on its own line per styleguide.
```suggestion
#httpsOnlyModeToggle,
#passwordsLeakToggleMove {
```
<settings-toggle-button id="passwordsLeakToggleMove"
Is there a benefit of using a different ID here? The two elements are mutuallly exclusive, no? Using `passwordsLeakToggle` would result in less necessary chanegs in other parts of the code I think.
Need to update the tests that exercise this toggle to also be triggered with `passwordLeakToggleMove: true`. Otherwise, the newly added toggle is not tested anywhere and what will happen with these tests once the feature launches? Will whoever cleans up the flag just delete these tests, or port them to the new toggle at that time?
Both of the above sound suboptimal to me.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. | Gerrit |
#httpsOnlyModeToggle, #passwordsLeakToggleMove {
Each selector should be on its own line per styleguide.
```suggestion
#httpsOnlyModeToggle,
#passwordsLeakToggleMove {
```
+1, and I'd recommend to craft the CSS selector so that it doesn't the toggle in the old position, and matches the toggle in the new position.
<settings-toggle-button id="passwordsLeakToggleMove"
Is there a benefit of using a different ID here? The two elements are mutuallly exclusive, no? Using `passwordsLeakToggle` would result in less necessary chanegs in other parts of the code I think.
+1, you should be able to use the same ID, and craft the CSS selectors to match when the element is in the position here, and to not match when the element is in the position further up.
Also: I find "Move" in this ID confusing, because the toggle itself is not about "moving" a password leak or so. Hence "move" is not descriptive for what the toggle does or represents. (IIUC, you probably mean "move" to refer to the feature being enabled moving the toggle to a different position - which is _not_ an attribute of the toggle itself.)
passwordLeakToggleMove_: {
Let's stick to using the |enable| prefix ([details](https://docs.google.com/document/d/1pJxZ80cetPGTjyOzc8TgL0nat2wLUXnpfTxuZWgIV2s/edit?resourcekey=0-ffcQ5e8VmHIp6yUsZ1QNrw&tab=t.0#heading=h.os7moxlhkdiz)).
Need to update the tests that exercise this toggle to also be triggered with `passwordLeakToggleMove: true`. Otherwise, the newly added toggle is not tested anywhere and what will happen with these tests once the feature launches? Will whoever cleans up the flag just delete these tests, or port them to the new toggle at that time?
Both of the above sound suboptimal to me.
+1, please follow the guidance in go/test-new-codepaths to enable the feature for the complete test file and to explicitly test both the new and old codepaths respectively.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. | Gerrit |