Refactored css to avoid need for sass, postcss and autoprefixer #264
Conversation
There was a problem hiding this comment.
This looks good to me. I have one comment about documenting where the vendored highlight.js came. Unless @sir-dunxalot has any objections, I will merge it after that!
| @@ -0,0 +1,34 @@ | |||
| /* Colors */ | |||
|
|
|||
| :root { | |||
There was a problem hiding this comment.
I believe CSS variables here without PostCSS might kinda break the demo site on IE11, but I think that's probably fine.
There was a problem hiding this comment.
I hope that the second commit should help with this — the variables now only override the default colour scheme so on IE it should display OK but without code highlighting.
There was a problem hiding this comment.
Am fine with using css variables. I'd be surprised if any devs are using IE11 😛
ember-cli-build.js
Outdated
| @@ -37,5 +16,9 @@ module.exports = function(defaults) { | |||
| behave. You most likely want to be modifying `./index.js` or app's build file | |||
| */ | |||
|
|
|||
| app.import('vendor/highlight.pack.js', { | |||
There was a problem hiding this comment.
For posterity's sake, what were the options used for generating this? Might be worth adding a comment above.
|
Thanks @maxfierke I've added the comment explaining the customization. |
tests/dummy/app/styles/app.css
Outdated
| h3 { | ||
| border-bottom: 1px solid #ccc; | ||
| } | ||
| .ember-popover h3, p { |
There was a problem hiding this comment.
just noticed this: should probably be .ember-popover p, right?
|
@maxfierke thanks! well spotted 👍 |
| height: 10px; | ||
| margin: -5px; | ||
| background: inherit; | ||
| transform: rotate(45deg); |
There was a problem hiding this comment.
Main thing to be aware of with the removal of the autoprefixer dep is we drop support for a couple of lesser-used old browsers. To be extra safe, perhaps we can add in -webkit prefixes for transform and transition rules. However, I don't really mind either way since:
- This is already supporting graceful fallbacks
- We can deal with this if and when an issue is reported
So I'll leave the addition of prefixes up to @chrism's discretion.
There was a problem hiding this comment.
Hi @sir-dunxalot, thanks for the review.
My understanding is that transform & transition only remain unsupported by IE8 and opera mobile.
Using a prefix (such as -webkit) wouldn't solve this for those two browsers (would require polyfilling I think) and as Ember 2.0 itself no longer supports IE8 I'd hoped it wouldn't be necessary to support it in this addon.
Also, another consideration is that as the transform is located in a pseudo element selector (:after) I don't think IE8 supports that anyway.
If I've got some of this wrong I'd be happy to update, but I don't think there is much possible for the reasons I've listed.
Thanks.
|
@maxfierke @chrism LGTM. 👍 |
As per my slack question this is a PR which hopefully addresses issues relating to SASS compilation.
For me, the main one was the missing source map error message which was caused upstream by
ember-cli-sass#76 & #113But I think it could also simplify a lot of the issues people have had in general working with the styling.
Initially I just flattened the selectors in the
addon.scssfile to remove the need to use any processing and converted it to a css file.My plan was to then just use processing for the styling of the dummy app.
Unfortunately this wasn't sufficient to use
ember-cli-sasspurely as a dev dependency (because theaddon.scssfile gets automatically included in the pipeline by default whenember-cli-sassis installed).Looking at the styling for the dummy app it showed that SASS was only necessary for some nesting of selectors (and some color variables).
I used the same approach as with the addon.scss file to flatten the selectors and by using native CSS variables it removed the need for SASS completely.
My rationale is that as this styling isn't even used by the addon, purely for the tests and documentation it would be acceptable to require modern browser support.
Doing this allowed me to remove 3 styling addons which are no longer necessary
ember-cli-sassautoprefixerember-cli-postcssAdditionally, in the course of updating the files I noticed that the
ember-code-snippetaddon was injecting some CSS into theaddon.cssthat wasn't being used by the addon and actually conflicted with the extra CSS from_code.scssin the dummy app.The current live documentation http://sir-dunxalot.github.io/ember-tooltips/ seems to have some inconsistent code styling and I feel like this might have been a contributing factor.
So I took the opportunity to also clean this up a bit by removing all that unused styling code and then implemented a simple colour scheme that works with the handlebars code (see example below).
I also followed the recommendations in the documentation to not use the default
highlight.jsfile and instead a custom one.The tests all still pass and I've also tested it in an existing app and a new app without any issues.
I'm hoping you agree that this will make the use of this addon more straightforward and reduces the chances of issues due to the styling pipeline.