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

Avoid Sass dependency #257

Open
samselikoff opened this issue Aug 9, 2018 · 33 comments
Open

Avoid Sass dependency #257

samselikoff opened this issue Aug 9, 2018 · 33 comments

Comments

@samselikoff
Copy link
Collaborator

Believe vanilla app.css can @import 'ember-modal-dialog.css' these days, we should lay out the path to removing the scss dependency and providing the styles for any stack.

@chrism
Copy link
Contributor

chrism commented May 18, 2019

I completely agree with this and would be happy to do the work.

I did something similar for Ember Tooltips some time ago sir-dunxalot/ember-tooltips#264

Also, it seems like 3.0.0 would be a good moment to implement it?

Most basic approach

The SCSS could just be flattened into CSS so ember-modal-structure.scss becomes

.ember-modal-dialog {
  z-index: 51;
  position: fixed;
}

.ember-modal-dialog.emd-in-place {
  position: static;
}

.ember-modal-wrapper.emd-static.emd-wrapper-target-attachment-center .ember-modal-dialog {
  top: 50%;
  left: 50%;
  transform: translate(-50%, -50%);
}

.ember-modal-wrapper.emd-animatable .ember-modal-dialog {
  position: relative;
}

.ember-modal-wrapper.emd-animatable.emd-wrapper-target-attachment-center {
  width: 100vw;
  height: 100vh;
  position: fixed;
  top: 0;
  left: 0;
  z-index: 50;
  display: flex;
  align-items: center;
  justify-content: center;
}

.ember-modal-wrapper.emd-animatable.emd-wrapper-target-attachment-center .ember-modal-overlay {
  display: flex;
  align-items: center;
  justify-content: center;
}

.ember-modal-overlay {
  width: 100vw;
  height: 100vh;
  position: fixed;
  top: 0;
  left: 0;
  z-index: 50;
}

and stays the same just changes to .css

.ember-modal-dialog {
  border-radius: 8px;
  background-color: #fff;
  box-shadow: 0 0 10px #222;
  padding: 10px;
}
.ember-modal-overlay.translucent {
  background-color: rgba(#808080, .77);
}

This would be enough to offer the option for non-SASS users as well and to include the css via

@import "ember-modal-dialog/ember-modal-structure.css";
@import "ember-modal-dialog/ember-modal-appearance.css";

without forcing the installation of ember-cli-sass.

More ambitious option

But it could also be taken further and to remove the files and the dev dependency for ember-cli-sass completely.

This would mean a breaking change, but a very simple one to fix (just add the extension to the css files).

It would also mean updating the CSS in the dummy app to no longer rely on SASS—which is something that I'd be happy to implement if it will be merged.

Then ember-cli-sass could be removed completely as a dependency.

Let me know if either option is acceptable and I will submit a PR.

Thanks.

@lukemelia
Copy link
Contributor

lukemelia commented May 20, 2019

Either option seems OK to me @samselikoff. Which would you recommend?

@lukemelia
Copy link
Contributor

Meant to ask your opinion @chrism.

@samselikoff
Copy link
Collaborator Author

If we can remove ember cli sass I say let’s go for it!

@chrism
Copy link
Contributor

chrism commented May 21, 2019

@lukemelia @samselikoff From my perspective the CSS is simple in both the dummy app and the dialog themselves so it's much more a question of whether we want to make what is a very small breaking change or not.

This is how it was presented in Ember Tooltips and didn't seem to have caused any problems.

Other Changes
SASS has been replaced in favor of plain ol' CSS. No more node-sass dependency! (#264 Thanks @chrism!)
If you were importing ember-tooltips SCSS into your app's stylesheets previously and you run into issues, you may need to remove the import.

I think at some point it will be necessary to migrate away from SCSS to CSS, so now might be the right moment?

The other option is to keep including the SCSS files, but that is going to involve duplication of files and more maintenance.

If it was my choice I'd say lets make the change now and I'll submit a PR!

Also, I've noticed that you use translate(-50%, -50%) to center the modal which causes some issues for me with blurring when this doesn't land on absolute pixels—I'd like to fix that too but think maybe its a separate issue and PR.

Just let me know your decision :)

@samselikoff
Copy link
Collaborator Author

Let's go ahead and modify the sass to be flattened vanilla css rules and provide instructions on how to import, and remove ember-cli-sass and the sass dependency 👍

@lukemelia
Copy link
Contributor

👍

@chrism
Copy link
Contributor

chrism commented May 22, 2019

FYI I've just submitted a PR updating the dependencies first, so I can then work on this.

@chrism
Copy link
Contributor

chrism commented May 24, 2019

I've just submitted a PR which replaces SCSS with CSS completely.
#278

Doing this and updating the test dummy app made me wonder though... would it be better to switch to https://ember-learn.github.io/ember-cli-addon-docs/ for the documentation now too?

@greyhwndz
Copy link

greyhwndz commented May 31, 2019

@lukemelia will this update the addon to a pure css one? I hope this push thru coz ember-cli-sass is giving me problems with my ember-cli-postcss setup. Thanks @chrism @samselikoff !!!

@chrism
Copy link
Contributor

chrism commented May 31, 2019

Hi @greyhwndz that was my motivation for making this change, yes.

It removes the need for SASS completely both in your client app and in the addon itself.

@greyhwndz
Copy link

@chrism I just hope this gets merged by @lukemelia soon coz I don't want to use ember-cli-sass just because an addon needs it and I won't be using sass in my app

@chrism
Copy link
Contributor

chrism commented Jun 1, 2019

@greyhwndz you are in luck - it was merged yesterday! 😁

@greyhwndz
Copy link

@chrism I just installed it. I tried:

@import "ember-modal-dialog/ember-modal-structure.css";
@import "ember-modal-dialog/ember-modal-appearance.css";

in my app/styles/app.css
and it gave me the following error:

Failed to find 'ember-modal-dialog/ember-modal-structure'
  in [
    C:\Users\USER\AppData\Local\Temp\broccoli-168049jHNKtyrL5DQ\out-361-broccoli_merge_trees_full_application\app\styles,
        D:\@\tailwind-ui\node_modules
  ]

Am using postcss-import with ember-cli-postcss

@chrism
Copy link
Contributor

chrism commented Jun 1, 2019 via email

@chrism
Copy link
Contributor

chrism commented Jun 1, 2019

hi @greyhwndz so I've made a clean project to test with and this commit

chrism/test-modal-css@fdea121

Shows that those imports work on a fresh application without postcss-import and ember-cli-postcss.

I'm now going to see if I can work out what is going on with them even though I don't use them myself.

I see in your error message its looking for the files in @\tailwind-ui\node_modules — have you got some setup beyond postcss-import and ember-cli-postcss, like some tailwind configuration which could be useful to working out what the problem is?

@greyhwndz
Copy link

greyhwndz commented Jun 1, 2019

@chrism wow thanks for even trying! been on this for some days already
Here are the things that I did:

$ ember install ember-cli-postcss
$ npm install --save-dev tailwindcss
$ npm install --save-dev postcss-import
$ npm i -D @fullhuman/postcss-purgecss

./app/styles/app.css

@import "tailwindcss/base";
@import "tailwindcss/components";
@import "tailwindcss/utilities";
@import "tailwindcss/screens";
@import "ember-modal-dialog/ember-modal-structure";
@import "ember-modal-dialog/ember-modal-appearance";

$ npx tailwind init ./config/tailwind.config.js --full

./ember-cli-build.js

const EmberApp = require('ember-cli/lib/broccoli/ember-app');
const purgecss = require('@fullhuman/postcss-purgecss');

module.exports = function(defaults) {
  let app = new EmberApp(defaults, {
    postcssOptions: {
      compile: {
        plugins: [{
            module: require('postcss-import'),
            options: {
              path: ['node_modules']
            }
          },
          require("tailwindcss")("./config/tailwind.config.js")
        ]
      },
      filter: {
        enabled: false,
        plugins: [{
          module: purgecss,
          options: {
            content: ['./app/**/*.hbs', './app/**/*.js']
          }
        }]
      }
    }
  return app.toTree();
};

I disabled postcss-purgecss for the meantime coz it was causing a different problem...

@chrism
Copy link
Contributor

chrism commented Jun 1, 2019

@greyhwndz You're welcome. I wanted to first check to make sure that it was a problem relating to postcss, rather than an issue with the new approach of importing directly into app.css.

Now I can see what you are trying to do I completely empathise!

Not sure if you subscribe to Ember Map but some time ago there was a conversation there, which I was involved in, about trying to get purgeCSS working with ember-cli-tailwind addon at that time.

In the end I put it on hold as I couldn't get it working and wanted the benefits of ember-cli-tailwind configuration—but this has rekindled my interest in getting it working.

Honestly, I think this is probably more an issue with postcss configuration than a problem with using @import in app.css but I think it will be really useful to get right as I'm sure a lot of people are/will have the same issues.

All I can say is that I'm going to spend a little time now trying to see if I can get tailwind working like this myself and will report back if I have any success—thats the best I can promise I'm afraid.

@greyhwndz
Copy link

@chrism thanks for the response and the effort that you will try to do. are you on ember discourse?

@chrism
Copy link
Contributor

chrism commented Jun 1, 2019

@greyhwndz not often but I'm just taking a look at https://discuss.emberjs.com/t/postcss-import-problem-with-tailwindcss-v1-0/16595/3 now.

I was still at the Failed to find 'tailwindcss/base' error, so looks like am going down the same path you did.

@greyhwndz
Copy link

@chrism I solved that for the moment with:

plugins: [
          {
            module: require('postcss-import'),
            options: {
              path: ['node_modules']
            }
          },
          require("tailwindcss")("./config/tailwind.config.js")
        ]

and the tailwindcss/base now is found

@chrism
Copy link
Contributor

chrism commented Jun 1, 2019

Yeh, so I've got there too thanks to your posts and you know what...

I think the problem goes right back to the start.

In your app.css you are importing like this:

@import "tailwindcss/base";
@import "tailwindcss/components";
@import "tailwindcss/utilities";
@import "tailwindcss/screens";
@import "ember-modal-dialog/ember-modal-structure";
@import "ember-modal-dialog/ember-modal-appearance";

But now that the ember-modal-dialog are CSS files you need to append the .css to them.

When I've tried this...

@import "tailwindcss/base";
@import "tailwindcss/components";
@import "tailwindcss/utilities";
@import "tailwindcss/screens";
@import "ember-modal-dialog/ember-modal-structure.css";
@import "ember-modal-dialog/ember-modal-appearance.css";

It seems to work fine now for me... let me know please!

Not sure whether to try purge-css now or save that rabbit hole for another day...

@chrism
Copy link
Contributor

chrism commented Jun 1, 2019

For info: this is a commit to a working example with tailwind and ember-modal-dialog

chrism/test-modal-css@a548408

@greyhwndz
Copy link

@chrism save the purge-css rabbit hole for another day! :D
Ok will try appending .css

@greyhwndz
Copy link

@chrism the configuration you have is not working with me. Am back to "ember-modal-dialog/ember-modal-structure.css" is not found

@greyhwndz
Copy link

This one is working for me, the modal-dialog is displaying, but it made the links in my app unresponsive.

const EmberApp = require('ember-cli/lib/broccoli/ember-app');

module.exports = function(defaults) {
  let app = new EmberApp(defaults, {
    postcssOptions: {
      compile: {
        extension: ['scss'],
        enabled: true,
        parser: require('postcss-scss'),
        plugins: [{
            module: require('@csstools/postcss-sass'),
            options: {
              includePaths: [
                'node_modules'
              ]
            }
          },
          {
            module: require('postcss-import'),
            options: {
              path: ['node_modules']
            }
          },
          require("tailwindcss")("./config/tailwind.config.js")
        ]
      },
      filter: {
        enabled: false,
        plugins: [{
          module: require('@fullhuman/postcss-purgecss'),
          options: {
            content: ['./app/**/*.hbs', './app/**/*.js']
          }
        }]
      }
    }
  });

  return app.toTree();
};

I am working on the embermap functional css so ping me on the discourse if it would be convenient for you

@chrism
Copy link
Contributor

chrism commented Jun 1, 2019

Really?

So weird then. Can you clone this repo and see if it works for you please?

https://github.com/chrism/test-modal-css

On my machine it seems to work fine

Screenshot 2019-06-01 at 14 30 21

@greyhwndz
Copy link

Ah! no! you are right it is working now @chrism !

@chrism
Copy link
Contributor

chrism commented Jun 1, 2019

So I did go down that rabbit hole and it seems like purgecss is now working too @greyhwndz .

Here is the ember-cli-build.js configuration

// ember-cli-build.js
'use strict';

const EmberApp = require('ember-cli/lib/broccoli/ember-app');

module.exports = function(defaults) {
  let app = new EmberApp(defaults, {
    postcssOptions: {
      compile: {
        plugins: [
          {
            module: require('postcss-import'),
            options: {
              path: ['node_modules']
            }
          },
          require("tailwindcss")("./config/tailwind.config.js"),
          {
            module: require('@fullhuman/postcss-purgecss'),
            options: {
              content: [
                // extra paths here probably needed for components/controllers etc
                './app/templates/**/*.hbs'
              ]
            }
          }
        ]
      }
    }
  });
  return app.toTree();
};

and to make sure that the normalize and modal styles weren't getting stripped by purgecss I added this to the app.css

/* purgecss start ignore */
@import "ember-modal-dialog/ember-modal-structure.css";
@import "ember-modal-dialog/ember-modal-appearance.css";
@import "tailwindcss/base";
/* purgecss end ignore */

@import "tailwindcss/components";
@import "tailwindcss/utilities";

But it does seem to work, at least in development. 😁

I guess the next step is to now only purge in production and check that the minification/uglification still works.

You can see the working commit here:
chrism/test-modal-css@27698af

@greyhwndz
Copy link

Whoa! @chrism! Amazing exploration! Will do a new project later to replicate that coz in my current project just merely importing the ember-modal-dialog css...

@greyhwndz
Copy link

greyhwndz commented Jun 2, 2019

@chrism followed your setup and configuration on a new project but I am still having:

Build Error (PostcssCompiler)

Failed to find 'ember-modal-dialog/ember-modal-structure.css'
  in [
    C:\Users\USER\AppData\Local\Temp\broccoli-8516BlF6rtFOO3SI\out-142-broccoli_merge_trees_full_application\app\styles,
        D:\@\tester\node_modules
  ]

Could this be a windows-specific problem?

@chrism
Copy link
Contributor

chrism commented Jun 2, 2019

@greyhwndz can you share this new project repo please?

@greyhwndz
Copy link

greyhwndz commented Jun 2, 2019

This is now working on master!!! thanks to the code contributors and to @chrism for helping determining the problem!!! can be closed now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants