Closed Bug 1364099 Opened 8 years ago Closed 7 years ago

Cleanup autocomplete css

Categories

(DevTools :: Shared Components, enhancement)

Product:
DevTools
Component:
Shared Components
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox56 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: ntim, Assigned: ruturaj)

References

Details

Attachments

(1 file)

fix-1364099-1.patch
(deleted), patch
jdescottes
: review+
Details | Diff | Splinter Review
See bug 1354504 comment 41. We may want to remove code that adds the theme classes as well.
Hey Tim, Referring to bug 1354504 comment 38, The CSS isn't present perhaps got changed in some other commit. Can I know what the fix for this bug now changes to? Thanks
Flags: needinfo?(ntim.bugs)
(In reply to Ruturaj Vartak from comment #1) > Hey Tim, > Referring to bug 1354504 comment 38, The CSS isn't present perhaps got > changed in some other commit. FYI, the last commit which changed the CSS significantly was bug 1354504: https://hg.mozilla.org/mozilla-central/filelog/58c5151bfd62de934b2286dbd664e69886270e28/devtools/client/themes/common.css > Can I know what the fix for this bug now changes to? - Removing the code that adds the theme classes to the popup - Simplifying box-shadow and cursor rules You might want to ask Julian about this.
Flags: needinfo?(ntim.bugs)
Attached patch fix-1364099-1.patch (deleted) — — Splinter Review
Hey Julian from your comments below > 1 - We already have a box-shadow defined for .devtools-autocomplete-popup on line 45 (with a slightly different value 0 1px 0 hsla(209,29%,72%,.25) inset;) > 2 - I think this particular box-shadow here was only set for .devtools-autocomplete-popup in light-theme. Is there a good reason to also apply it to CodeMirror-hints & CodeMirror-Tern-tooltip? Both these classes already have box-shadows defined in light-theme.css and dark-theme.css > 3 - In case you need to target .devtools-autocomplete-popup in the light-theme only, you should know that the light-theme classname is also added to the container when we are using the Firebug theme (an easy way for the firebug theme to inherit from the light theme). That means that you either need to have a very specific .light-theme:not(.firebug-theme) in your selector, or to override the value for firebug-theme. > Now, I'm being very nit-picky here, for the sake of explaining the impact these kind of changes can have. We're talking about an almost invisible inset box shadow here, I don't think anybody would notice the difference between before and after your change :) So I don't really ask you to keep the colors 100% as they were before, let's just make sure we don't redefine the box-shadow property several times for no reason. To be honest I would just get rid of this box shadow: it's almost not noticeable, and when it is it feels dated. But it is easier if we do this kind of changes in a separate changeset. It seems removal of box-shadow is the only thing. Could you please have a look? Thanks, Rutu
Assignee: nobody → ruturaj
Attachment #8878769 - Flags: review?(jdescottes)
Comment on attachment 8878769 [details] [diff] [review] fix-1364099-1.patch Review of attachment 8878769 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch! Yes looks like this is the only thing left to update :)
Attachment #8878769 - Flags: review?(jdescottes) → review+
Keywords: checkin-needed
Thanks Julian
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/dceb915d3c40 Cleanup autocomplete css. r=jdescottes
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dceb915d3c40
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: