Closed
Bug 1484891
Opened 6 years ago
Closed 6 years ago
Make sidebar and sidebar_text properties apply to the sidebar header
Categories
(Firefox :: Theme, defect, P3)
Firefox
Theme
Tracking
()
RESOLVED
FIXED
Firefox 63
Tracking | Status | |
---|---|---|
firefox-esr60 | --- | unaffected |
firefox61 | --- | unaffected |
firefox62 | --- | unaffected |
firefox63 | - | fixed |
People
(Reporter: ntim, Assigned: ntim)
References
Details
(Whiteboard: [ntim-intern-project])
Attachments
(2 files, 3 obsolete files)
3.13 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
4.66 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
Bug 1418602 only makes them apply to the bookmarks and history trees.
Assignee | ||
Comment 1•6 years ago
|
||
Attachment #9003445 -
Flags: review?(dao+bmo)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•6 years ago
|
||
Attachment #9003446 -
Flags: review?(dao+bmo)
Assignee | ||
Comment 3•6 years ago
|
||
Attachment #9003449 -
Flags: review?(dao+bmo)
Assignee | ||
Comment 4•6 years ago
|
||
Attachment #9003451 -
Flags: review?(dao+bmo)
Assignee | ||
Updated•6 years ago
|
Attachment #9003445 -
Attachment is obsolete: true
Attachment #9003445 -
Flags: review?(dao+bmo)
Assignee | ||
Updated•6 years ago
|
Attachment #9003446 -
Attachment is obsolete: true
Attachment #9003446 -
Flags: review?(dao+bmo)
Assignee | ||
Updated•6 years ago
|
Component: Themes → Theme
Product: WebExtensions → Firefox
Whiteboard: [ntim-intern-project]
Updated•6 years ago
|
Priority: -- → P3
Comment 5•6 years ago
|
||
Comment on attachment 9003449 [details] [diff] [review] Consolidate browser sidebar styling >+#browser { >+ --sidebar-border-color: ThreeDShadow; Unless this is supposed to ever change at runtime, better use a preprocessor directive instead of a CSS variable.
Attachment #9003449 -
Flags: review?(dao+bmo)
Assignee | ||
Comment 6•6 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #5) > Comment on attachment 9003449 [details] [diff] [review] > Consolidate browser sidebar styling > > >+#browser { > >+ --sidebar-border-color: ThreeDShadow; > > Unless this is supposed to ever change at runtime, better use a preprocessor > directive instead of a CSS variable. I'm planning to allow changing the sidebar border in a different bug.
Flags: needinfo?(dao+bmo)
Assignee | ||
Comment 7•6 years ago
|
||
Bug 1418602 is incomplete without this bit.
status-firefox61:
--- → unaffected
status-firefox62:
--- → unaffected
status-firefox63:
--- → affected
status-firefox-esr60:
--- → unaffected
tracking-firefox63:
--- → ?
Updated•6 years ago
|
Flags: needinfo?(dao+bmo)
Attachment #9003449 -
Flags: review+
Comment 8•6 years ago
|
||
Comment on attachment 9003451 [details] [diff] [review] Make sidebar and sidebar_text properties apply to the sidebar header >+ ["--sidebar-background-color", { >+ lwtProperty: "sidebar", >+ optionalElementID: "sidebar-box", >+ processColor(rgbaChannels, element) { >+ if (!rgbaChannels) { >+ element.removeAttribute("lwt-sidebar"); >+ return null; >+ } >+ const {r, g, b, a} = rgbaChannels; >+ element.setAttribute("lwt-sidebar", "true"); >+ return `rgba(${r}, ${g}, ${b}, ${a})`; What's the background behind sidebar-box? Trying to figure out if the alpha channel makes sense here.
Assignee | ||
Comment 9•6 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #8) > Comment on attachment 9003451 [details] [diff] [review] > Make sidebar and sidebar_text properties apply to the sidebar header > > >+ ["--sidebar-background-color", { > >+ lwtProperty: "sidebar", > >+ optionalElementID: "sidebar-box", > >+ processColor(rgbaChannels, element) { > >+ if (!rgbaChannels) { > >+ element.removeAttribute("lwt-sidebar"); > >+ return null; > >+ } > >+ const {r, g, b, a} = rgbaChannels; > >+ element.setAttribute("lwt-sidebar", "true"); > >+ return `rgba(${r}, ${g}, ${b}, ${a})`; > > What's the background behind sidebar-box? Trying to figure out if the alpha > channel makes sense here. It's whatever background is set on #main-window, so it probably doesn't make sense. Will remove it in the next iteration of the patch.
Assignee | ||
Comment 10•6 years ago
|
||
Attachment #9005816 -
Flags: review?(dao+bmo)
Assignee | ||
Updated•6 years ago
|
Attachment #9003451 -
Attachment is obsolete: true
Attachment #9003451 -
Flags: review?(dao+bmo)
Comment 11•6 years ago
|
||
Comment on attachment 9005816 [details] [diff] [review] Make sidebar and sidebar_text properties apply to the sidebar header > ["--lwt-sidebar-background-color", { > lwtProperty: "sidebar", >+ processColor(rgbaChannels) { >+ if (!rgbaChannels) { >+ return null; >+ } >+ const {r, g, b} = rgbaChannels; >+ // Drop alpha channel >+ return `rgb(${r}, ${g}, ${b})`; >+ } Can we add support for something like 'opaque: true' in the variable map to get rid of the code duplication for dropping alpha channels?
Assignee | ||
Comment 12•6 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #11) > Comment on attachment 9005816 [details] [diff] [review] > Make sidebar and sidebar_text properties apply to the sidebar header > > > ["--lwt-sidebar-background-color", { > > lwtProperty: "sidebar", > >+ processColor(rgbaChannels) { > >+ if (!rgbaChannels) { > >+ return null; > >+ } > >+ const {r, g, b} = rgbaChannels; > >+ // Drop alpha channel > >+ return `rgb(${r}, ${g}, ${b})`; > >+ } > > Can we add support for something like 'opaque: true' in the variable map to > get rid of the code duplication for dropping alpha channels? Good idea, filed bug 1487991 to do this.
Updated•6 years ago
|
Attachment #9005816 -
Flags: review?(dao+bmo) → review+
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 13•6 years ago
|
||
Pushed by ntim.bugs@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d24dc1dba873 Consolidate browser sidebar styling. r=dao https://hg.mozilla.org/integration/mozilla-inbound/rev/9a7e99501586 Make sidebar and sidebar_text properties apply to the sidebar header. r=dao
Keywords: checkin-needed
Comment 14•6 years ago
|
||
Pushed by csabou@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/dbd19d423cde Fix eslint failure.
Comment 15•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d24dc1dba873 https://hg.mozilla.org/mozilla-central/rev/9a7e99501586 https://hg.mozilla.org/mozilla-central/rev/dbd19d423cde
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•