Skip to content

Comments

Fix EmptyContent Reactivity#3090

Merged
skjnldsv merged 1 commit intomasterfrom
fix/emptyContent_reactivity
Aug 23, 2022
Merged

Fix EmptyContent Reactivity#3090
skjnldsv merged 1 commit intomasterfrom
fix/emptyContent_reactivity

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 <[email protected]>
@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
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