Draft: fix(addon-mobile): correct sheet dialog theme-color handling#14515
Draft: fix(addon-mobile): correct sheet dialog theme-color handling#14515loxy wants to merge 1 commit into
Conversation
BundleMonUnchanged files (2)
Total files change +2B +0.01% Groups updated (1)
Final result: ✅ View report in BundleMon website ➡️ |
Tests completed successfully ✅Good job 🔥 |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
Hi @waterplea — this is the v5 PR bundling the three theme-color fixes you greenlit in discussion #14124. Refs #13690. Marked draft on purpose: the fixes are done and covered (jest + a Cypress test), but three API-shape calls are yours to make before I finalize:
One heads-up from on-device testing: the address-bar ( |
0111b64 to
277b13c
Compare
Three independent failure modes in TuiSheetDialogService / TuiThemeColorService around `<meta name="theme-color">` handling: - Capture the theme color at the moment the first sheet opens instead of once at construction, so a theme change made after the service was created (e.g. an app-level dark-mode effect) is no longer lost on revert, and nested sheets no longer revert onto the sheet color. - Make the sheet color configurable via `themeColor` on TUI_SHEET_DIALOG_OPTIONS (defaults to #404040, `null` opts out) instead of a hardcoded constant that only matched the backdrop in light mode. - Split TuiThemeColorService: `color` now writes only the `<meta name="theme-color">` tag, while the new `navColor` setter writes the `--tui-theme-color` CSS variable used by navigation chrome. Dimming the address bar no longer flickers the nav. Adds a jest spec (revert / nesting / opt-out matrix), a Cypress test for the real-browser open/close behavior, and a `themeColor` docs example.
277b13c to
1ea8dc0
Compare
waterplea
left a comment
There was a problem hiding this comment.
This looks good, thank you very much! I think I only have couple of concerns:
- Maybe let's not change the
TuiThemeColorServiceand handle metadata directly inTuiSheetDialogServiceusingMeta? - We can probably not introduce null and just use empty string as falsy value for opt-out? If you prefer more expressive
nullwe can still use!themeColorin checks since empty string also makes no sense as color - What's the purpose of
const {themeColor} = this.options;? Seems like an extra line, can we just usethis.options.themeColor?
|
Thank you for the feedback. I'm on vacation currently. Don't know how much time I will have next days. But I will try to address your suggestions and remarks. |
Three independent failure modes in TuiSheetDialogService / TuiThemeColorService around
<meta name="theme-color">handling:themeColoronTUI_SHEET_DIALOG_OPTIONS(defaults to#404040,nullopts out) instead of a hardcoded constant that only matched the backdrop in light mode.colornow writes only the<meta name="theme-color">tag, while the newnavColorsetter writes the--tui-theme-colorCSS variable used by navigation chrome. Dimming the address bar no longer flickers the nav.Adds a jest spec (revert / nesting / opt-out matrix), a Cypress test for the real-browser open/close behavior, and a
themeColordocs example.Fixes #13690