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)

defect

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)

Bug 1418602 only makes them apply to the bookmarks and history trees.
Blocks: 1385518
Attachment #9003445 - Flags: review?(dao+bmo)
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
Attachment #9003449 - 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.
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

Created:
Updated:
Size: