If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

revise styling of bookmark editing panel

VERIFIED FIXED in fennec1.0b5

Status

Fennec Graveyard
Bookmarks
P1
normal
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: madhava, Assigned: mfinkle)

Tracking

(Blocks: 1 bug)

Trunk
fennec1.0b5
x86
Mac OS X
Dependency tree / graph

Details

(Whiteboard: [polish])

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

8 years ago
Created attachment 402167 [details]
suggested style revision

See the attachment.
(Reporter)

Updated

8 years ago
tracking-fennec: --- → ?
Whiteboard: [polish]
Assignee: nobody → 21
Depends on: 517411
Blocks: 521561
(Reporter)

Updated

8 years ago
Priority: -- → P1

Updated

8 years ago
tracking-fennec: ? → 1.0+
Created attachment 407306 [details] [diff] [review]
wip v0.1

Dirty wip for hildon.
Created attachment 407307 [details]
screenshot
Created attachment 407449 [details] [diff] [review]
patch

This patch is like Vivien's, but moves the textbox changes out and uses a true 40px/24px image
Assignee: 21 → mark.finkle
Attachment #407306 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #407449 - Flags: review?(gavin.sharp)
     this._editor.setAttribute("id", "bookmark-item");
+    this._editor.setAttribute("class", "panel-dark-all");
     this._editor.setAttribute("flex", "1");

I promise to remove this unneeded change

Comment 5

8 years ago
Created attachment 407826 [details]
Starred - 40x40

Comment 6

8 years ago
Created attachment 407827 [details]
Starred - 40x40
Attachment #407826 - Attachment is obsolete: true
Comment on attachment 407449 [details] [diff] [review]
patch

Looks like you forgot to hg add starred-24.png?

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js

>     this._editor = document.createElement("placeitem");
>     this._editor.setAttribute("id", "bookmark-item");
>+    this._editor.setAttribute("class", "panel-dark-all");
>     this._editor.setAttribute("flex", "1");

I don't understand by what you mean by "remove this unneeded change". Are you going to remove all the panel-dark-all additions to hildon's browser.css as well?

>diff --git a/themes/hildon/platform.css b/themes/hildon/platform.css

>+.panel-dark-all {
>+.panel-dark-all button,
>+.panel-dark-all button:not([disabled="true"]):active:hover,
>+.panel-dark-all button[disabled="true"] {

>diff --git a/themes/hildon/browser.css b/themes/hildon/browser.css

> /* bookmark editor   ------------------------------------------------------- */
> #bookmark-container {
>   padding: 1mm; /* core spacing */
>+  -moz-box-shadow: black 0 0.25mm 0.25mm;
> }

Should bookmark-container just be a dialog-dark instead?

>+#bookmark-form placeitem {
>+  background-color: transparent;

Should this just be the default?

>+#bookmark-form .bookmark-controls {
>   display: none;
> }

Belongs in the content CSS, arguably.
Attachment #407449 - Flags: review?(gavin.sharp) → review+
(In reply to comment #7)
> (From update of attachment 407449 [details] [diff] [review])
> Looks like you forgot to hg add starred-24.png?

Found some odd naming. I renamed starred-40.png to star-40.png and used the existing star-24.png for the low res image

> >     this._editor = document.createElement("placeitem");
> >     this._editor.setAttribute("id", "bookmark-item");
> >+    this._editor.setAttribute("class", "panel-dark-all");
> >     this._editor.setAttribute("flex", "1");
> 
> I don't understand by what you mean by "remove this unneeded change". Are you
> going to remove all the panel-dark-all additions to hildon's browser.css as
> well?

I had the "panel-dark-all" crap in there originally, but saw Vivien used "transparent" as a way around my problem. It's nicer so I used it, but forgot to yank the old code.Yes, I removed it from everywhere.

> > #bookmark-container {
> >   padding: 1mm; /* core spacing */
> >+  -moz-box-shadow: black 0 0.25mm 0.25mm;
> > }
> 
> Should bookmark-container just be a dialog-dark instead?

Maybe someday. Currently, dialog-dark is used for centered dialogs with rounded corners. The bookmark-editor is tucked up under the urlbar like larry panel.

> >+#bookmark-form placeitem {
> >+  background-color: transparent;
> 
> Should this just be the default?

Yeah, moved to content CSS

> >+#bookmark-form .bookmark-controls {
> >   display: none;
> > }
> 
> Belongs in the content CSS, arguably.

It could, but I have been shying away from putting explicit (by id) rules in content CSS for now.
pushed:
https://hg.mozilla.org/mobile-browser/rev/7972f6374613
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → B5
verified FIXED On builds:

Mozilla/5.0 (X11; U; Linux armv7l; en-US; rv:1.9.2b1pre) Gecko/20091026
Fennec/1.0b5pre

and

Mozilla/5.0 (X11; U; Linux armv6l; en-US; rv:1.9.3a1pre) Gecko/20091026
Fennec/1.0b5pre
Status: RESOLVED → VERIFIED
Component: General → Bookmarks
You need to log in before you can comment on or make changes to this bug.