[DSBD] Update style for the enterprise signals disclaimer ui [chromium/src : main]
Stanisław Solarewicz (Gerrit)
Stanisław Solarewicz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Stanisław Solarewicz
| Commit-Queue | +1 |
1 comment
Hello, could you please take a look?
Nicolas - the enterprise parts
Sasha - the frontend parts
Thanks!
Related details
- Nicolas Ouellet-Payeur
- Sasha Kunitskaya
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sasha Kunitskaya (Gerrit)
Sasha Kunitskaya added 5 comments![]( "Open in Gerrit")
thanks, looks good overall, i've left a few comments (not a full review yet)
<g id="account-circle" viewBox="0 -960 960 960">
<path
d="M234-276q51-39 114-61.5T480-360q69 0 132 22.5T726-276q35-41 54.5-93T800-480q0-133-93.5-226.5T480-800q-133 0-226.5 93.5T160-480q0 59 19.5 111t54.5 93Zm146.5-204.5Q340-521 340-580t40.5-99.5Q421-720 480-720t99.5 40.5Q620-639 620-580t-40.5 99.5Q539-440 480-440t-99.5-40.5ZM480-80q-83 0-156-31.5T197-197q-54-54-85.5-127T80-480q0-83 31.5-156T197-763q54-54 127-85.5T480-880q83 0 156 31.5T763-763q54 54 85.5 127T880-480q0 83-31.5 156T763-197q-54 54-127 85.5T480-80Zm100-95.5q47-15.5 86-44.5-39-29-86-44.5T480-280q-53 0-100 15.5T294-220q39 29 86 44.5T480-160q53 0 100-15.5ZM523-537q17-17 17-43t-17-43q-17-17-43-17t-43 17q-17 17-17 43t17 43q17 17 43 17t43-17Zm-43-43Zm0 360Z">
</path>
</g>is this needed?
${this.showSignalsDisclaimer_ ? html`
<managed-user-profile-notice-disclosure-device-signalsQuestion: I see a slight naming inconsistency:
- `managed-user-profile-notice-disclosure-device-signals` as element name
- `SignalsDisclaimer` in .ts (methods/css class name/in `State` in browser_proxy)
- `DEVICE_SIGNALS_DISCLAIMER` in `ScreenType` in browser_proxy
- `deviceSignals...Label` in strings in managed_user_profile_notice_ui.cc
Is it intended? Does it make sense to unify them to sth like "SignalsDisclaimer"/"DeviceSignalsDisclaimer" everywhere?
font-family: 'Google Sans Text', Roboto;here and below: remove `font-family` and `font-style` (these properties are inherited and have no effect here)
also please check if `font-weight` and `line-height` across the file have effect. They may have the values you set manually by default for given elements.
border-radius: 24px;nit: sort properties alphabetically across the file (I use AI for this, as I work in Cider and it doesn't format css files automatically by one button, as other files)
Related details
- Nicolas Ouellet-Payeur
- Stanisław Solarewicz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sasha Kunitskaya (Gerrit)
Sasha Kunitskaya added 5 comments![]( "Open in Gerrit")
I think first round is complete from my side :) thanks!
<g id="account-circle" viewBox="0 -960 960 960">
<path
d="M234-276q51-39 114-61.5T480-360q69 0 132 22.5T726-276q35-41 54.5-93T800-480q0-133-93.5-226.5T480-800q-133 0-226.5 93.5T160-480q0 59 19.5 111t54.5 93Zm146.5-204.5Q340-521 340-580t40.5-99.5Q421-720 480-720t99.5 40.5Q620-639 620-580t-40.5 99.5Q539-440 480-440t-99.5-40.5ZM480-80q-83 0-156-31.5T197-197q-54-54-85.5-127T80-480q0-83 31.5-156T197-763q54-54 127-85.5T480-880q83 0 156 31.5T763-763q54 54 85.5 127T880-480q0 83-31.5 156T763-197q-54 54-127 85.5T480-80Zm100-95.5q47-15.5 86-44.5-39-29-86-44.5T480-280q-53 0-100 15.5T294-220q39 29 86 44.5T480-160q53 0 100-15.5ZM523-537q17-17 17-43t-17-43q-17-17-43-17t-43 17q-17 17-17 43t17 43q17 17 43 17t43-17Zm-43-43Zm0 360Z">
</path>
</g>is this needed?
upd: i see, it's used in html file
assertNotReached();I think it'd be safer to use `return this.i18n('deviceSignalsContinueLabel');`
In general for `managed_user_profile_notice_app_refresh` the idea is to copy the behavior of the original `managed_user_profile_notice_app`, as with the `refresh` feature flag on it's fully replacing it.
It's not the best for adding changes, to duplicate them across two files, but I think it may be the correct option currently...
You can test locally with `--enable-features=FirstRunDesktopRefresh` to see if it works.
return html`<!--_html_template_start_-->wrap in `// clang-format off ` ` // clang-format on`,
https://chromium.googlesource.com/chromium/src/+/HEAD/docs/webui/webui_lit_style_guide.md#formatting
this.i18n('deviceSignalsDisclosureTitle')}</h1>nit: I guess better to use double indentation here: "Indent 4 spaces when wrapping a previous line."
source https://chromium.googlesource.com/chromium/src/+/HEAD/docs/webui/webui_lit_style_guide.md#formatting
Nicolas Ouellet-Payeur (Gerrit)
Nicolas Ouellet-Payeur added 3 comments![]( "Open in Gerrit")
display: flex;
flex-direction: column;Is this actually needed? This is basically "lay out elements vertically", but that's already how HTML works by default. Or am I missing something?
position: relative;How confident are you that all of these CSS attrs are needed? It seems like a lot of the layout/positioning attrs are redundant. TBF it's always hard to tell just by reading the CSS
.disclaimer .icon {optional nit: since Chrome 120 we have CSS nesting. Feel free to use it! within reason :)
```
.disclaimer {
align-items: center;
...
&:first-child { ... }
h2 { ... }
p { ... }
}
```btw you can filter based on "outside context" too e.g.
```
.disclaimer-container {
...
&:first-child { ... }
.profile-picker & { /* same as `.profile-picker .disclaimer-container` */ }
.modal-dialog & { /* same as `.modal-dialog .disclaimer-container` */ }
}
```Related details
- Stanisław Solarewicz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stanisław Solarewicz (Gerrit)
Stanisław Solarewicz added 10 comments![]( "Open in Gerrit")
<g id="account-circle" viewBox="0 -960 960 960">
<path
d="M234-276q51-39 114-61.5T480-360q69 0 132 22.5T726-276q35-41 54.5-93T800-480q0-133-93.5-226.5T480-800q-133 0-226.5 93.5T160-480q0 59 19.5 111t54.5 93Zm146.5-204.5Q340-521 340-580t40.5-99.5Q421-720 480-720t99.5 40.5Q620-639 620-580t-40.5 99.5Q539-440 480-440t-99.5-40.5ZM480-80q-83 0-156-31.5T197-197q-54-54-85.5-127T80-480q0-83 31.5-156T197-763q54-54 127-85.5T480-880q83 0 156 31.5T763-763q54 54 85.5 127T880-480q0 83-31.5 156T763-197q-54 54-127 85.5T480-80Zm100-95.5q47-15.5 86-44.5-39-29-86-44.5T480-280q-53 0-100 15.5T294-220q39 29 86 44.5T480-160q53 0 100-15.5ZM523-537q17-17 17-43t-17-43q-17-17-43-17t-43 17q-17 17-17 43t17 43q17 17 43 17t43-17Zm-43-43Zm0 360Z">
</path>
</g>Sasha Kunitskayais this needed?
upd: i see, it's used in html file
Exactly, this is used for the Personal information details card.
${this.showSignalsDisclaimer_ ? html`
<managed-user-profile-notice-disclosure-device-signalsQuestion: I see a slight naming inconsistency:
- `managed-user-profile-notice-disclosure-device-signals` as element name
- `SignalsDisclaimer` in .ts (methods/css class name/in `State` in browser_proxy)
- `DEVICE_SIGNALS_DISCLAIMER` in `ScreenType` in browser_proxy
- `deviceSignals...Label` in strings in managed_user_profile_notice_ui.cc
Is it intended? Does it make sense to unify them to sth like "SignalsDisclaimer"/"DeviceSignalsDisclaimer" everywhere?
Good point, I updated the code to unify under `signalsDisclaimer`.
assertNotReached();I think it'd be safer to use `return this.i18n('deviceSignalsContinueLabel');`
In general for `managed_user_profile_notice_app_refresh` the idea is to copy the behavior of the original `managed_user_profile_notice_app`, as with the `refresh` feature flag on it's fully replacing it.
It's not the best for adding changes, to duplicate them across two files, but I think it may be the correct option currently...
You can test locally with `--enable-features=FirstRunDesktopRefresh` to see if it works.
I'd much rather keep this assertion. The signals_disclaimer is not meant to be shown within the refreshed UI. At the moment the signals disclaimer is always shown with the old UI URL so it should never hit the assertion. If the assertion is hit in the future we will know that someone is doing something wrong.
Let's remove this assertion once we explicitly decide to show the signals disclaimer on the refreshed UI. What do you think?
I've run with `--enable-features=FirstRunDesktopRefresh` nothing happend.
here and below: remove `font-family` and `font-style` (these properties are inherited and have no effect here)
also please check if `font-weight` and `line-height` across the file have effect. They may have the values you set manually by default for given elements.
Thanks for pointing this out! `font-weight` and `line-height` do seem to have an effect so I leave them as is.
Is this actually needed? This is basically "lay out elements vertically", but that's already how HTML works by default. Or am I missing something?
Indeed, please see my comment about redundant properties.
position: relative;How confident are you that all of these CSS attrs are needed? It seems like a lot of the layout/positioning attrs are redundant. TBF it's always hard to tell just by reading the CSS
Not confident at all! I simply copied the existing css for the disclaimer and added my margins, colors and fonts.
Prompted by your comment I have tasked Ai with getting rid of the redundant styles and the visual results remains unchanged, while a bulk has been removed.
nit: sort properties alphabetically across the file (I use AI for this, as I work in Cider and it doesn't format css files automatically by one button, as other files)
Done
optional nit: since Chrome 120 we have CSS nesting. Feel free to use it! within reason :)
```
.disclaimer {
align-items: center;
...&:first-child { ... }
h2 { ... }
p { ... }
}
```btw you can filter based on "outside context" too e.g.
```
.disclaimer-container {
...&:first-child { ... }
.profile-picker & { /* same as `.profile-picker .disclaimer-container` */ }
.modal-dialog & { /* same as `.modal-dialog .disclaimer-container` */ }
}
```
Hey that's pretty cool! Makes css much more readable. Implemented this for the profile-picker/modal-dialog distinctions.
wrap in `// clang-format off ` ` // clang-format on`,
https://chromium.googlesource.com/chromium/src/+/HEAD/docs/webui/webui_lit_style_guide.md#formatting
Done
nit: I guess better to use double indentation here: "Indent 4 spaces when wrapping a previous line."
source https://chromium.googlesource.com/chromium/src/+/HEAD/docs/webui/webui_lit_style_guide.md#formatting
- Nicolas Ouellet-Payeur
- Sasha Kunitskaya
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |