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: make the library compatible with AoT and universal, dont bundle stylesheets #551

Closed
wants to merge 1 commit into from

Conversation

mattlewis92
Copy link
Collaborator

@mattlewis92 mattlewis92 commented Aug 31, 2017

Same changes as in Gbuomprisco/ng2-material-dropdown#37, with ng2-material-dropdown pointed to a temporary fork until that PR can be reviewed and merged.

BREAKING CHANGE:
The libraries stylesheet must now be included somewhere in your app. For CLI users this is a case of adding this line to src/styles.css:

@import '~ngx-chips/dist/ngx-chips.css';

Fixes #550, #537, #497, #492, #451

@codecov
Copy link

codecov bot commented Aug 31, 2017

Codecov Report

Merging #551 into master will decrease coverage by 0.27%.
The diff coverage is 74.79%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #551      +/-   ##
==========================================
- Coverage   64.56%   64.28%   -0.28%     
==========================================
  Files          16       16              
  Lines       32035    31821     -214     
  Branches      164      164              
==========================================
- Hits        20683    20456     -227     
- Misses      11264    11277      +13     
  Partials       88       88
Impacted Files Coverage Δ
modules/tag-input.module.ts 58.33% <0%> (ø) ⬆️
...ponents/tag-input-form/tag-input-form.component.ts 81.81% <100%> (ø) ⬆️
modules/components/tag/tag.component.ts 55.17% <46.66%> (ø) ⬆️
modules/components/icon/icon.ts 50% <75%> (ø) ⬆️
...omponents/dropdown/tag-input-dropdown.component.ts 64.23% <80%> (ø) ⬆️
modules/components/tag-input/tag-input.ts 77.92% <83.33%> (ø) ⬆️
spec-bundle.js 64.12% <0%> (-0.29%) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b7acbe0...7c7f595. Read the comment docs.

…stylesheets

BREAKING CHANGE:
The libraries stylesheet must now be included somewhere in your app. For CLI users this is a case of adding this line to `src/styles.css`:
```
@import '~ngx-chips/dist/ngx-chips.css';
```
@xtianus79
Copy link

Ahhh you good sir!!!! I might be loving you today

@mattlewis92
Copy link
Collaborator Author

Happy to help 😄 If you want to use this now you can npm i @mattlewis92/ngx-chips

@xtianus79
Copy link

@mattlewis92 the only thing I question is the taking out the inlining of the stylesheets. but before i make judegment i have a question.

I used the scss mixin's to create a custom template. lol what is going to happen to that now?

@mattlewis92
Copy link
Collaborator Author

My reasoning behind that was to make it easier to override the CSS - in my app I have to use !important on every css rule I was to overwrite. The scss is still there and you can import it into your app to create your own theme. In fact that might actually decrease your bundle size a bit as then you only import what you need.

@xtianus79
Copy link

cool let me give it a spin... yes that is why i used the mixins an deep as to not have to override. if you nest your css to a more specific order than the style you usually won't have to do !important i.e.

 this > dom > tree > element > style class {
    background: blue;
} 

will beat

style class {
  ...

@mattlewis92
Copy link
Collaborator Author

That's true, but that feels even hackier + more verbose than using !important. Also by inlining the stylesheets into the components you wouldn't be able to use sass and hence its mixins. This is the only option I can see that will work 😞

@xtianus79
Copy link

i am getting this error with your repo

Could not resolve ngx-chips relative to /Users/xxxx/xxx/xxxxx/frontend/src/app/app.module.ts.

@Gbuomprisco
Copy link
Owner

Hi @mattlewis92, thanks for taking the time to work on this. The objections I have are related to the amount of changes needed for this, and practices I don't agree with like using any, or inlining everything in components. I am quite sure we can find a way to fix the issue without these changes. As for the breaking change, that would require a version bump too.

TL;DR: I am not going to merge this for now. I'll have a look this weekend to see if we can fix this in another way. But thanks!

@mattlewis92
Copy link
Collaborator Author

I absolutely agree with you that I wish there was a better way to do this. If you wish to keep the template + style sheet urls (and having the lib easily consumable and just work without any additional config by the end user) you will have to go down the route of post processing your template and metadata files to inline the sources. This is what the material team do in order for it to work for them:
https://github.com/angular/material2/blob/master/tools/package-tools/inline-resources.ts
https://github.com/angular/material2/blob/master/tools/package-tools/metadata-inlining.ts

In my opinion, that adds even more unnecessary complexity that what I've proposed in this PR, for the sake of just inlining templates and having the user include a stylesheet.

If you want to discuss more let me know, I'd love to work together with you on this to find an elegant solution 😄

@xtianus79
Copy link

@mattlewis92 I have used your repo and got my build working AOT with minimal effort (some css went a little wonky). Nevertheless, I definatly appreciate your hard work. I will say this at least puts everyone in the right direction.

Thanks @Gbuomprisco for looking into this and hopefully something more concrete comes along this weekend for you.

@xtianus79
Copy link

@mattlewis92 hey buddy. I have your fork installed and was wondering why the close icon had dissapeared? Is that something I need to pull in i.e. like the fonts?

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.

None yet

3 participants