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

variable width using css custom properties #7

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

fujeffrey1
Copy link

No description provided.

@fujeffrey1 fujeffrey1 mentioned this pull request Dec 7, 2019
Copy link
Member

@antony antony left a comment

Choose a reason for hiding this comment

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

Looks good apart from some minor tweaks. Out of interest, how is browser compatibility for CSS vars?

src/Notifications.svelte Outdated Show resolved Hide resolved
@fujeffrey1 fujeffrey1 requested a review from antony December 8, 2019 17:10
@ahopkins
Copy link

ahopkins commented Dec 9, 2019

Out of interest, how is browser compatibility for CSS vars?

@antony Quite good in my opinion. Definitely acceptable in for my work.

image

@antony
Copy link
Member

antony commented Dec 9, 2019

Out of interest, how is browser compatibility for CSS vars?

@antony Quite good in my opinion. Definitely acceptable in for my work.

Unfortunately lack of support for IE11 is a dealbreaker. What options do we have regarding the support for legacy (but not dead) browsers?

@fujeffrey1
Copy link
Author

The simplest way is to try using this polyfill: https://jhildenbiddle.github.io/css-vars-ponyfill/#/

Otherwise, I can also look into other workarounds/implementations.

@fujeffrey1
Copy link
Author

I also got it to work with this: https://stackoverflow.com/a/31868716/11268067

It looks more hacky, but doesnt require any polyfills/libraries.

@antony
Copy link
Member

antony commented Dec 9, 2019

Short of adding ponyfills (note, it is indeed a ponyfill, you have to run it for each new element), which seems like overkill for one property, I think we'd need to look at a more static way of doing this for one very specific reason - with the code as it is now, if you were to create a 20vw notification and then an 80vw one, it would resize all elements currently on the screen when it appeared.

@fujeffrey1
Copy link
Author

Short of adding ponyfills (note, it is indeed a ponyfill, you have to run it for each new element), which seems like overkill for one property, I think we'd need to look at a more static way of doing this for one very specific reason - with the code as it is now, if you were to create a 20vw notification and then an 80vw one, it would resize all elements currently on the screen when it appeared.

I agree that could be an issue but it seems like a very specific edge case. I can't imagine a use case where a NotificationDisplay component would be created dynamically to trigger this. I also didn't plan for multiple NotificationDisplays because the existing code doesn't really support this (notify would notify all components, they would all be positioned in the top right and overlap, etc).

@antony
Copy link
Member

antony commented Dec 10, 2019

@fujeffrey1 I'm not talking about multiple NotificationDisplay components, I'm saying, if you create a custom notification size it will resize all other notifications which already exist on the screen, since it resizes the container.

The code definitely doesn't support multiple NotificationDisplay components, that would (at least per my original plan) defeat the purpose.

@mehdiraized
Copy link

i need this in desktop very bad show notification toast

@antony
Copy link
Member

antony commented May 5, 2020

@mehdiraized then have a think about how we can work around the issue mentioned above and we can progress the PR.

@antony
Copy link
Member

antony commented May 25, 2021

In a few months we can merge this, since IE11 will be truly dead!

Copy link
Member

@antony antony left a comment

Choose a reason for hiding this comment

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

Coming back to this, I think this would be better implemented as a style prop: https://svelte.dev/docs#template-syntax-component-directives---style-props

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