Open
Bug 1443217
Opened 7 years ago
Updated 2 years ago
Merge duplicated CSS variables from common.inc.css
Categories
(Toolkit :: Themes, enhancement, P3)
Toolkit
Themes
Tracking
()
NEW
People
(Reporter: jaws, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
/toolkit/themes/shared/in-content/common.inc.css defines some variables that have similar names and are defined with the same value. These variables are never used with different values, so they can be merged.
--in-content-page-color
--in-content-text-color
│
└─ Replace *-text-color with *-page-color to match *-page-background
--in-content-border-color
--in-content-box-border-color
│
└─ Replace *-box-border-color with *-border-color to match *-border-focus/highlight
Updated•7 years ago
|
Reporter | ||
Updated•7 years ago
|
Assignee: nobody → abishekhmjee
Status: NEW → ASSIGNED
This is the merged file: https://pastebin.com/MfyqH5rW
Reporter | ||
Comment 2•7 years ago
|
||
Please submit your patch through MozReview and request review from me by putting "r?jaws" at the end of the commit message. You can follow the steps here to learn how to push to MozReview: https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html#submitting-commits-for-review
Flags: needinfo?(abishekhmjee)
Updated•7 years ago
|
Priority: -- → P3
Comment hidden (mozreview-request) |
Reporter | ||
Updated•7 years ago
|
Flags: needinfo?(abishekhmjee)
Reporter | ||
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8967853 [details]
Bug 1443217 - Merge duplicated CSS variables from common.inc.css
https://reviewboard.mozilla.org/r/236554/#review242810
::: browser/config/mozconfig:10
(Diff revision 1)
> # This file specifies the build flags for Firefox. You can use it by adding:
> # . $topsrcdir/browser/config/mozconfig
> # to the top of your mozconfig file.
>
> ac_add_options --enable-application=browser
> + # Automatically download and use compiled C++ components:
Please revert this change.
::: toolkit/themes/shared/in-content/common.inc.css:10
(Diff revision 1)
> %endif
> @namespace html "http://www.w3.org/1999/xhtml";
> @namespace xul "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
>
> *|*:root {
> - --in-content-page-color: #0c0c0d;
> + --in-content-page-text-category-text-active-color: #0c0c0d;
nit, please remove any trailing white space at the end of these lines.
::: toolkit/themes/shared/in-content/common.inc.css:18
(Diff revision 1)
> --in-content-box-background-odd: #f3f6fa;
> --in-content-box-background-hover: #ebebeb;
> --in-content-box-background-active: #dadada;
> - --in-content-box-border-color: #d7d7db;
> + --in-content-box-border-color: #d7d7db;
> --in-content-item-hover: rgba(0,149,221,0.25);
> - --in-content-item-selected: #0a84ff;
> + --in-content-item-selected-border-highlight-focus-text-selected-header-button-background: #0a84ff;
This doesn't seem right. Did you try doing a find-and-replace multiple times without checking the output?
Attachment #8967853 -
Flags: review?(jaws) → review-
Comment on attachment 8967853 [details]
Bug 1443217 - Merge duplicated CSS variables from common.inc.css
https://reviewboard.mozilla.org/r/236554/#review242810
> This doesn't seem right. Did you try doing a find-and-replace multiple times without checking the output?
Yes, I tried find and replace tool multiple times, couldn't check the output.I actually didn't know how to check the output, any specified tool out there?
Thanks
Comment hidden (mozreview-request) |
Reporter | ||
Comment 7•6 years ago
|
||
mozreview-review |
Comment on attachment 8986295 [details]
Bug 1443217 - Merge duplicated CSS variables from common.inc.css.
https://reviewboard.mozilla.org/r/251668/#review258062
It looks like this commit is based on top of your previous commit. Can you fold them together? You can run `hg histedit` then choose the `fold` option on the newest commit to fold it in to the previous commit. Then you can repush your patch for review.
Attachment #8986295 -
Flags: review?(jaws) → review-
Comment hidden (mozreview-request) |
Reporter | ||
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8986352 [details]
Bug 1443217 - Merge duplicated CSS variables from common.inc.css
https://reviewboard.mozilla.org/r/251694/#review258218
Please fold all your changesets together.
Attachment #8986352 -
Flags: review?(jaws) → review-
Reporter | ||
Comment 10•6 years ago
|
||
(In reply to Apratim from comment #5)
> Comment on attachment 8967853 [details]
> Bug 1443217 - Merge duplicated CSS variables from common.inc.css
>
> https://reviewboard.mozilla.org/r/236554/#review242810
>
> > This doesn't seem right. Did you try doing a find-and-replace multiple times without checking the output?
>
> Yes, I tried find and replace tool multiple times, couldn't check the
> output.I actually didn't know how to check the output, any specified tool
> out there?
> Thanks
Well, in this case you could just look at the file and see the output of the find-and-replace. You can also use `hg diff` to look at the changes and verify that they make sense. You can also build Firefox, and look at the various parts of the user interface that use these variables and make sure that their appearance hasn't changed from a build that doesn't have your changes.
Comment hidden (mozreview-request) |
Attachment #8967853 -
Attachment is obsolete: true
Attachment #8986295 -
Attachment is obsolete: true
Reporter | ||
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8986352 [details]
Bug 1443217 - Merge duplicated CSS variables from common.inc.css
https://reviewboard.mozilla.org/r/251694/#review258360
::: toolkit/themes/shared/in-content/common.inc.css:18
(Diff revision 2)
> --in-content-box-background-odd: #f3f6fa;
> --in-content-box-background-hover: #ebebeb;
> --in-content-box-background-active: #dadada;
> --in-content-box-border-color: #d7d7db;
> --in-content-item-hover: rgba(0,149,221,0.25);
> - --in-content-item-selected: #0a84ff;
> + --in-content-item-selected-border-highlight-focus-text-selected-header-button-background: #0a84ff;
This variable name doesn't make any sense. You've got your commits folded together now, but the issues I commented on in my first review haven't been addressed.
Attachment #8986352 -
Flags: review?(jaws) → review-
Reporter | ||
Comment 13•6 years ago
|
||
This webpage has guides to help you build Firefox,
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build#Building_Firefox_for_the_Desktop
I would recommend using an Artifact build as it will build much faster since you don't need to make any C++ changes,
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Artifact_builds
Updated•6 years ago
|
Assignee: abishekhmjee → nobody
Status: ASSIGNED → NEW
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•