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 EmptyContent Reactivity #3090

Merged
merged 1 commit into from
Aug 23, 2022
Merged

Fix EmptyContent Reactivity #3090

merged 1 commit into from
Aug 23, 2022

Conversation

jotoeri
Copy link
Contributor

@jotoeri jotoeri commented Aug 23, 2022

So - this fixes Reactivity for me.

Before After
186104852-f83290cf-5ca7-4754-848a-86d62e503ded.mp4
186104885-81e2b846-7d93-40c1-9e51-a0a041aaf99f.mp4

(With this code: https://github.com/nextcloud/forms/pull/1308/files)

@Pytal can you remember, why you introduced that reactivity stuff in #2867 ? Any place, where i can/need to test it?

@raimund-schluessler As you mentioned https://github.com/nextcloud/tasks/blob/0611f1b897c4681035b7e6541247dd4dcc9cea44/src/components/TaskCreateDialog.vue#L77-L85 - Does this work for you on current master? Do you see similar issues as me above (Not seeing the description on second EmptyContent)?

@jotoeri jotoeri added 3. to review Waiting for reviews feature: emptycontent Related to the emptycontent component labels Aug 23, 2022
@raimund-schluessler
Copy link
Contributor

@raimund-schluessler As you mentioned https://github.com/nextcloud/tasks/blob/0611f1b897c4681035b7e6541247dd4dcc9cea44/src/components/TaskCreateDialog.vue#L77-L85 - Does this work for you on current master? Do you see similar issues as me above (Not seeing the description on second EmptyContent)?

With nc/vue master from yesterday evening (before #3082 was merged), my example from the Tasks app works as it should. The second NcEmptyContent correctly shows up.

@jotoeri
Copy link
Contributor Author

jotoeri commented Aug 23, 2022

With nc/vue master from yesterday evening (before #3082 was merged), my example from the Tasks app works as it should. The second NcEmptyContent correctly shows up.

Hmm. @raimund-schluessler Would you mind testing nextcloud/forms#1308 in your setup? 🙈

It is about the Empty-Contents, that are visible on the App-root localhost/index.php/apps/forms

Copy link
Contributor

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

This drops the text check for title :/

@jotoeri
Copy link
Contributor Author

jotoeri commented Aug 23, 2022

This drops the text check for title :/

Turning it into a prop makes it a breaking change for v6, but brings back validation?

@jotoeri jotoeri force-pushed the fix/emptyContent_reactivity branch from 2d66dea to 6d85dd0 Compare August 23, 2022 14:57
@skjnldsv
Copy link
Contributor

This drops the text check for title :/

Turning it into a prop makes it a breaking change for v6, but brings back validation?

Seems more fitting than a slot yes! 👍

@jotoeri jotoeri added the breaking PR that requires a new major version label Aug 23, 2022
@jotoeri jotoeri added this to the 6.0.0 milestone Aug 23, 2022
Signed-off-by: Jonas Rittershofer <jotoeri@users.noreply.github.com>
@jotoeri jotoeri force-pushed the fix/emptyContent_reactivity branch from 6d85dd0 to 9442870 Compare August 23, 2022 15:03
@skjnldsv skjnldsv requested a review from a team August 23, 2022 15:19
@skjnldsv skjnldsv merged commit 8d4c1e5 into master Aug 23, 2022
@skjnldsv skjnldsv deleted the fix/emptyContent_reactivity branch August 23, 2022 17:26
@Pytal
Copy link
Contributor

Pytal commented Aug 23, 2022

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 breaking PR that requires a new major version feature: emptycontent Related to the emptycontent component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants