-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Next: useUiNotification #5382
Next: useUiNotification #5382
Conversation
Pull Request Test Coverage Report for Build 469251453
💛 - Coveralls |
💙 vsf-next-demo successfully deployed at https://b3613b123406e55fadc9f4beca2b23c69f06c524.vsf-next-demo.preview.storefrontcloud.io |
I have removed the |
0856c3a
to
577988d
Compare
Just like we talked about, I have removed |
packages/core/nuxt-theme-module/theme/components/Notification.vue
Outdated
Show resolved
Hide resolved
packages/core/nuxt-theme-module/theme/components/Notification.vue
Outdated
Show resolved
Hide resolved
packages/core/nuxt-theme-module/theme/components/Notification.vue
Outdated
Show resolved
Hide resolved
packages/core/nuxt-theme-module/theme/composables/useUiNotification/index.ts
Outdated
Show resolved
Hide resolved
|
||
return { | ||
send, | ||
notifications: computed(() => state.notifications) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder...
Shouldn't we put here [...state.notifications]
?
Currently, computed returns reference to the Heap. So probably it is still possible to modify an array via methods like push etc.
Wdyt @andrzejewsky @filrak ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets leave it like this
packages/core/nuxt-theme-module/theme/composables/useUiNotification/index.ts
Outdated
Show resolved
Hide resolved
packages/core/nuxt-theme-module/theme/composables/useUiNotification/index.ts
Outdated
Show resolved
Hide resolved
cc1e40b
to
a97841c
Compare
Related Issues
closes #5363
Short Description and Why It's Useful
Screenshots of Visual Changes before/after (if There Are Any)
Nagranie.z.ekranu.2021-01-8.o.14.42.06.mp4
Nagranie.z.ekranu.2021-01-8.o.14.46.57.mp4
Which Environment This Relates To
Check your case. In case of any doubts please read about Release Cycle
develop
branch and want to merge it back todevelop
release
branch and want to merge it back torelease
hotfix
ormaster
branch and want to merge it back tohotfix
Upgrade Notes and Changelog
IMPORTANT NOTICE
CHANGELOG.md
with description of your changeContribution and Currently Important Rules Acceptance