Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #150 - Fix colour editor for use in browsers that don't use -webkit- prefix #158

Merged
merged 1 commit into from
Apr 24, 2015

Conversation

AJDBenner
Copy link

Fix #150. This is what the fix looks like in Firefox.

screen shot 2015-04-01 at 1 43 14 pm

@@ -199,10 +208,13 @@
.color-editor section .slider.opacity-slider {
background-image: url("../img/color_thumb_back.png");
background-size: 8px;
margin-right: 0px;
margin-right: 0px;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert this change, please. Spurious whitespace-only change are to be avoided, since they make blame info harder to untangle.

@humphd
Copy link

humphd commented Apr 1, 2015

Please fix issues I've pointed out above. How does this look in IE9/10?

@le717, any thoughts? This change seems like something we should just do upstream, to be honest. I can't imagine there will be a lot of resistence, since it won't affect the -webkit-* case at all.

-moz-font-smoothing: antialiased;
-o-font-smoothing: antialiased;
-ms-font-smoothing: antialiased;
font-smoothing: antialiased;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is font-smoothing even a valid (read: non-WebKit only) property, and do all these prefixes even exist? The closest I can find is a font-smooth property from a very old (2002) CSS3 draft, and it has long been dropped from the spec.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. We have https://developer.mozilla.org/en-US/docs/Web/CSS/font-smooth in Firefox, but it's not really recommended. Probably we should just revert this section of the patch, and leave it unchanged.

@le717
Copy link

le717 commented Apr 1, 2015

@humphd Aside from the few notes, I see no harm in landing this upstream, especially since it does not effect us at all (if anything, it may be good as it switches us to use the standard gradient syntax). I'm not sure if an issue won't be raised about the prefixes, though.

@AJDBenner AJDBenner force-pushed the issue150 branch 3 times, most recently from f918e1d to 7f63ffc Compare April 7, 2015 15:15
@@ -88,7 +88,7 @@ define(function (require, exports, module) {
this.$opacitySlider = this.$element.find(".opacity-slider");
this.$opacitySelector = this.$element.find(".opacity-slider .selector-base");
this.$swatches = this.$element.find(".swatches");

console.log("LOOK HERE", this.$opacityGradient.selector);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this, please.

@@ -88,7 +88,6 @@ define(function (require, exports, module) {
this.$opacitySlider = this.$element.find(".opacity-slider");
this.$opacitySelector = this.$element.find(".opacity-slider .selector-base");
this.$swatches = this.$element.find(".swatches");

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert this change

@humphd
Copy link

humphd commented Apr 7, 2015

You should collect screenshots of this running in all the browsers, and also test it in Desktop Brackets. Then we should rebase against Brackets' master and do a PR upstream.

@le717
Copy link

le717 commented Apr 7, 2015

You should collect screenshots of this running in all the browsers, and also test it in Desktop Brackets. Then we should rebase against Brackets' master and do a PR upstream.

Just be sure to note the question about ordering in https://github.com/humphd/brackets/pull/158#discussion_r27601425.

@AJDBenner AJDBenner force-pushed the issue150 branch 2 times, most recently from 04740f6 to 1d8d9b5 Compare April 7, 2015 17:22
@AJDBenner
Copy link
Author

I've compiled screenshots for all the browsers.

Chrome
chrome

Firefox
firefox

Internet Explorer 10
ie10

Opera
opera

Safari
safari

Desktop Brackets
desktopbrackets

@AJDBenner
Copy link
Author

Just be sure to note the question about ordering in #158 (comment).

@le717 on desktop Brackets linear-gradient is used, no prefixes.

@AJDBenner
Copy link
Author

I filed this upstream adobe#10853

@AJDBenner AJDBenner force-pushed the issue150 branch 2 times, most recently from f77d6fa to ac9d939 Compare April 24, 2015 15:54
@gideonthomas gideonthomas merged commit dc361e1 into mozilla:bramble Apr 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants