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

Not working on IE since 5.0.0-0 at least #261

Closed
laura-bergoens opened this issue Jan 22, 2020 · 26 comments · Fixed by #262
Closed

Not working on IE since 5.0.0-0 at least #261

laura-bergoens opened this issue Jan 22, 2020 · 26 comments · Fixed by #262

Comments

@laura-bergoens
Copy link

Hi @mansona,

We've been using this great component in our project for a bit now.
We were using the 4.3.3 until now. We've updated to the latest version, and since then
the notifications do not display anymore on IE 11.
It's because of the var() property now being used in your component's css. Indeed, var() is not a supported property on IE.

My question is : have you officially drop support on IE or not ?

I'm obviously not requesting here that you make it happen (it's a lot of work, and let's be honest, IE...). I just want to know if you drop it or not, so we can adjust.

Thanks a lot, and thanks for the great work !

@cah-brian-gantzler
Copy link

This is an issue for us as well. If it is only the var? You could re-write the css not using the var, doesn't look like to much an issue, otherwise I think Im left with forking the repo and doing it myself until a more permanent solution can be made.

@laura-bergoens
Copy link
Author

laura-bergoens commented Jan 27, 2020

@cah-briangantzler That's what we did, we wrote a css specifically for IE. It wasn't much trouble

@mansona
Copy link
Owner

mansona commented Jan 27, 2020

Hey folks 👋 sorry for not responding to the issue here but I have been investigating this on-and-off since you reported it @laura-bergoens (thanks again for letting me know)

This seems to be a quirk of postcss and ember addons 🤔I essentially was hoping that nobody using this addon would notice any of the internal things with postcss but it seems like there has been some leakage in this particular issue as well as issues like this #260

The good news is that this can be fixed. The idea would be that we make use of the preserve option on postcss-preset-env plugin that we're using and also sidestep the automatic inclusion of the addon.css file in vendor css (pre-processed version) and instead only have the IE11 compatible post-processed version make its way to vendor.css from this addon.

I have finally got my local dev environment setup to be able to test this in IE11 and hopefully I will have a solution soon 😄

@laura-bergoens
Copy link
Author

@mansona Super cool ! Don't be sorry for not responding right away, Git issues are meant to be asynchronous discussions ;)

Thanks again ! I'm looking forward getting your solution. In the meantime, we can override existing css for IE (it's not that much of a stretch)

Thanks for your reactivity !

@cah-brian-gantzler
Copy link

Any update on this, going to be having a release coming up and need to know if I have to fix this myself temporarily.

Also since I believe it was the var statement, I saw this mentioned https://www.npmjs.com/package/css-vars-ponyfill . Not sure at all if that would help.

@mansona
Copy link
Owner

mansona commented Feb 11, 2020

So it took me a little longer than expected but it is working in the end 🎉 can you both try out ember-cli-notifications@next (version 6.1.0-0 prerelease) and see if it solves things for you?

@laura-bergoens
Copy link
Author

@mansona Hey, thanks for the release.
I'm getting this error when upgrading to 6.1.0-0 when running ember serve:
screenshot_2020-02-11_16:48:50

@cah-brian-gantzler
Copy link

Yea, sadly Im getting that as well. Going to have to look into forking and somehow fixing, got a release pending

@mansona
Copy link
Owner

mansona commented Feb 13, 2020

I should be able to fix this today, I know what the issue is and I know how to test it locally 👍

@mansona
Copy link
Owner

mansona commented Feb 13, 2020

It looks like I was able to fix it quicker than I expected 😂before work starts!

can you try v6.1.0-1 and see if it fixes it for you? you can always install these beta releases using npm i ember-cli-notifications@next 👍

@cah-brian-gantzler
Copy link

That does at least show the notification without erroring. However the notification is the entire width of the screen. Height looks maybe correct, hard to tell

@kienhaw
Copy link

kienhaw commented Feb 14, 2020

Hi, i have upgraded to 6.1.0-1 and the notifications (which has been missing after upgraded to v5/v6) seems to be fixed. But there comes another issue where it seems like the css has haywired the bootstrap/custom css. Please refer to screenshot below.

Before upgrading to 6.1.0-1 (4.3.3)
Screenshot 2020-02-14 at 3 49 22 PM

After upgraded to 6.1.0-1
Screenshot 2020-02-14 at 3 47 03 PM

P/s: I'm using latest version of chrome browser (80.0.3987.106)

@laura-bergoens
Copy link
Author

For someone who doesn't use boostrap, it's all good ! Feel free to close the issue as resolved, or wait until those boostrap/notif conflicts are resolved. However, for me it is solved ! Thanks for your work <3

laura-bergoens pushed a commit to 1024pix/pix that referenced this issue Feb 14, 2020
After issuing a regression on their repo :
mansona/ember-cli-notifications#261
the person in charge of developing and maintaining
ember-cli-notifications did his best to fix.
Problem was ember-cli-notif were not working anymore
on IE. We had to write a custom css for that issue.
Now it's not necessary.
@mansona
Copy link
Owner

mansona commented Feb 14, 2020

@cah-briangantzler are you sure that it's something in the ember-cli-notifications CSS that is causing this? can you inspect the element and let me know where the CSS is coming from?

@mansona
Copy link
Owner

mansona commented Feb 14, 2020

@kienhaw again are you sure that this new rule is coming from ember-cli-notifications? is your app public somewhere that I could take a look? is this just in IE or is it in other browsers too?

@cah-brian-gantzler
Copy link

cah-brian-gantzler commented Feb 14, 2020

Ill have to do that when I get home.

It worked perfectly when I forked the repo and did the following commit. cah-brian-gantzler@21ffe81

I would assume that the new css being provided however it is would somewhat reproduce end end result of the css from this commit

@mansona
Copy link
Owner

mansona commented Feb 18, 2020

ok folks! I have finally figured it all out 🎉

Firstly @kienhaw thanks for pointing this out, it actually ended up being an issue with me copy-pasting code from a different project into here 🙈 it's fixed now.

@cah-briangantzler can you try ember-cli-notifications@next now? it should install the next pre-release v6.2.0-0 which should fix your issues.

For anyone that is interested, essentially it seems like the postcss "polyfill" for css custom properties is not able to implement "scoped" variables correctly because of the way that the DOM works. So if we are supporting IE with the fallback syntax essentially all custom properties will be ignored 😫 you can read up more on postcss/postcss-custom-properties#1

My fix was to move all my custom properties back into the :root scope and prefix them with --ecn- which means that they will probably no longer clash with your existing custom properties @cah-briangantzler (which was probably what was causing this issue).

We're almost there folks! if nobody has any issues that they raise with v6.2.0-0 then I will merge #262 early next week

@kien-fple
Copy link

kien-fple commented Feb 19, 2020

@mansona Thanks for the new release (v6.2.0-0), it has fixed the css issue. Cheers mate! Great work there!

@cah-brian-gantzler
Copy link

cah-brian-gantzler commented Feb 19, 2020

It is no longer stretching the width of the screen. It now looks like there is no padding or margin.Just a thin block the height of the text in it. I was unable to get a screen shot. Will as soon as I can.

Going to check with people and see if that is acceptable for the IE people, working towards not having to support them so wont matter eventually. Appreciate your work.

FYI I also use boot strap V3

@mansona
Copy link
Owner

mansona commented Feb 19, 2020

@cah-briangantzler so it's tough to figure out if this is something that I'm doing wrong somewhere 🤔 I have tried to issolate all of my CSS from the application CSS by adding a prefix (manually) to all classes and now I've added a prefix to all custom properties so it really should be isolated!

If you could inspect the element and tell me if the styles that usually deal with the width/height of the notification are getting overridden by your own styles that would really help me out (and verify that it's not me doing something wrong on my end). Otherwise I don't know if I can diagnose this any more from afar unless I had access to the app to see what is going wrong 🤔

@cah-brian-gantzler
Copy link

Totally understand, will get you that, didnt have time when I was checking it.

@cah-brian-gantzler
Copy link

The screen shot I tried to take was terrible. But I think I see the problem.

For the classes .ember-cli-notifications-notificationcontainer .c-notificationcontent when it works the it has padding: .5rem 1rem; but on ie11 it has padding: var(--ecn-spacing-1) var(--ecn-spacing-2) . That is using 6.2.0-0

@cah-brian-gantzler
Copy link

When I compile and run in Chrome, targeting IE 11 still see that, but chrome must know how to read it and thats why it works

image

I am assuming that should not be what the css looks like when targeting IE11.

@mansona
Copy link
Owner

mansona commented Feb 24, 2020

@cah-briangantzler I'm curious why you're seeing this issue and I'm not 🤔 I have run the ember-cli-notifications dummy app locally and it is using the correct spacing values in IE11. I wonder if it is perhaps something in your build 🤔 can you go to https://deploy-preview-262--ember-cli-notifications.netlify.com/ in IE11 on your end and click the "show notification" button. (Ignore the style of the documentation page, the notifications should be correct)

@mansona
Copy link
Owner

mansona commented Feb 24, 2020

@cah-briangantzler also seeing as it seems to now only be broken for you how do you feel about opening a new issue and that means I can release v6.2.0 with the fixes for everyone else?

I'm happy to wait and get this right if you think it's best 😄

@cah-brian-gantzler
Copy link

The above url looks like for me.

Since its only the padding missing, I deployed using 6.2.0-0.

Yes, close this issue and release a good copy. Ill try to look at it see why. Might have to ask you a couple questions on how you are deploying the css now in order to see if there is a problem in my build.

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 a pull request may close this issue.

5 participants