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

revert: fix(NcActions): use new slots api #5196

Merged
merged 1 commit into from Feb 1, 2024

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Jan 31, 2024

☑️ Resolves

$scopedSlots cannot be used in beforeUpdated hook.

  • On re-rendering, when vnode is reused, on beforeUpdated state $scopedSlots still returns an old value and breaks rendering.
  • Officially, $slots/$scopedSlots should be used only during the rendering and not supposed to be used in data, computed or life cycle hooks.

This commit returns $slots to have an old behavior and resolve a regression

TODO: find a better solution to support both slots API. Use method in template and always call it, like we do in Vue 3?

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable

- $scopedSlots cannot be used in beforeUpdated.
  On re-rendering, when vnode is reused, on beforeUpdated $scopedSlots returns an old value and breaks rendering.
- Officially, $slots/$scopedSlots should be used only during the rendering
- This commit returns $slots to have an old behavior and resolve regression
- TODO: find better soluton to support both slots API

Reverts commit b94ff04

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme ShGKme added bug Something isn't working 3. to review Waiting for reviews feature: actions Related to the actions components regression Regression of a previous working feature labels Jan 31, 2024
@ShGKme ShGKme added this to the 8.6.1 milestone Jan 31, 2024
@ShGKme ShGKme self-assigned this Jan 31, 2024
Copy link

codecov bot commented Jan 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (83d844d) 39.34% compared to head (08cc694) 39.34%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5196   +/-   ##
=======================================
  Coverage   39.34%   39.34%           
=======================================
  Files         139      139           
  Lines        3688     3688           
  Branches      810      810           
=======================================
  Hits         1451     1451           
  Misses       2021     2021           
  Partials      216      216           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@susnux
Copy link
Contributor

susnux commented Jan 31, 2024

Also breaking Forms 🙈

@susnux
Copy link
Contributor

susnux commented Jan 31, 2024

Officially, $slots/$scopedSlots should be used only during the rendering and not supposed to be used in data, computed or life cycle hooks.

This also feels more natural to me, but not sure we can refactor like this. At least not non-breaking I guess.

@ShGKme
Copy link
Contributor Author

ShGKme commented Jan 31, 2024

This also feels more natural to me, but not sure we can refactor like this. At least not non-breaking I guess.

We can make text and isLongText methods and call them during the rendering. Then $scopedSlots in Vue 2 and $slots in Vue 3 will work fine. But it will have a small performance impact. These small slots will be rendered 3 times each rendering. Should be fine though (they just render a plain text).

An alternative could be a renderless component to cache a value during the rendering. This will work, but could be even less performant with an additional component instance =D

@ShGKme
Copy link
Contributor Author

ShGKme commented Jan 31, 2024

@susnux What do you think about #5197 ?

@susnux
Copy link
Contributor

susnux commented Jan 31, 2024

@susnux What do you think about #5197 ?

I think thats a viable solution 🎉
While I still think we should not have messed with the default slot, but if we do some special text handling, we should have required it to be passed as a prop.

@GretaD GretaD merged commit 6f1a4c9 into master Feb 1, 2024
20 checks passed
@GretaD GretaD deleted the fix/nc-actions--re-rendering-slots-issue branch February 1, 2024 09:14
@st3iny
Copy link
Contributor

st3iny commented Feb 1, 2024

Thanks for the quick fix @ShGKme ♥️

@ShGKme
Copy link
Contributor Author

ShGKme commented Feb 1, 2024

Thanks for the quick fix @ShGKme ♥️

Sorry for the regression 🙈

@susnux susnux mentioned this pull request Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug Something isn't working feature: actions Related to the actions components regression Regression of a previous working feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Action menu on envelope and threads is broken
4 participants