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)

Product:
Firefox
Component:
Theme
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
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)

Consolidate browser sidebar styling
(deleted), patch
dao
: review+
Details | Diff | Splinter Review
Make sidebar and sidebar_text properties apply to the sidebar header
(deleted), patch
dao
: review+
Details | Diff | Splinter Review
Bug 1418602 only makes them apply to the bookmarks and history trees.
Blocks: 1385518
Attached patch Consolidate browser sidebar styling (obsolete) (deleted) — — Splinter Review
Attachment #9003445 - Flags: review?(dao+bmo)
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
Attachment #9003446 - Flags: review?(dao+bmo)
Attachment #9003449 - Flags: review?(dao+bmo)
Attachment #9003451 - Flags: review?(dao+bmo)
Attachment #9003445 - Attachment is obsolete: true
Attachment #9003445 - Flags: review?(dao+bmo)
Attachment #9003446 - Attachment is obsolete: true
Attachment #9003446 - Flags: review?(dao+bmo)
Component: Themes → Theme
Product: WebExtensions → Firefox
Whiteboard: [ntim-intern-project]
Priority: -- → P3
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)
(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)
Bug 1418602 is incomplete without this bit.
status-firefox61: --- → unaffected
status-firefox62: --- → unaffected
status-firefox63: --- → affected
status-firefox-esr60: --- → unaffected
tracking-firefox63: --- → ?
Flags: needinfo?(dao+bmo)
Attachment #9003449 - Flags: review+
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.
(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.
Attachment #9003451 - Attachment is obsolete: true
Attachment #9003451 - Flags: review?(dao+bmo)
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?
(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.
Attachment #9005816 - Flags: review?(dao+bmo) → review+
Keywords: checkin-needed
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
Depends on: 1488118
Blocks: 1488000
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: