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

feat(reference): Add opt-in for interactive mode and render widgets lazy once in view #5257

Merged
merged 4 commits into from
Mar 5, 2024

Conversation

juliushaertl
Copy link
Contributor

@juliushaertl juliushaertl commented Feb 19, 2024

  • Make sure we only render widgets that are within the visible viewport
  • Allow to opt-in to interactive mode if requested by apps
  • Moved API to typescript as requested by @skjnldsv
  • Interactive widgets are default, but opt out possible if needed (e.g. for talk sidebar) and the option to make interaction opt in)

☑️ Resolves

  • Fix #…

🖼️ Screenshots

Kapture.2024-02-19.at.17.09.06.webm

🚧 Tasks

  • ...

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 3️⃣ Backport to next requested with a Vue 3 upgrade

@juliushaertl
Copy link
Contributor Author

juliushaertl commented Feb 19, 2024

@jancborchardt The user facing adjustment here would be that @nickvergessen suggested to make interactivity a opt in variant in apps where having scroll containers might be disturbing by default (like in Talk when you scroll through the chat history and would just want to go through that without your scroll being catched by some interactive widget passing by). I tried to accomplish that in a generic way so that it is independent from individual implementations of the widgets.

Kapture.2024-02-19.at.21.22.40.webm

Note that apps showing the widget can decide whether they want interactivity by default or on user confirmation per widget. We don't persist this so it is needed to click on every page load per widget. The button is shown on hover or keyboard focus.

What do you think?

@jancborchardt
Copy link
Contributor

The user facing adjustment here would be that @nickvergessen suggested to make interactivity a opt in variant in apps where having scroll containers might be disturbing by default (like in Talk when you scroll through the chat history and would just want to go through that without your scroll being catched by some interactive widget passing by).
Note that apps showing the widget can decide whether they want interactivity by default or on user confirmation per widget.

So e.g. in Text it a Table would be shown by default embedded, while in Talk it would be shown with the simpler view?

Because my worry is that the simpler view makes the feature difficult to discover. Currently the way of embedding is decided by the embedder, with the default being "interactive" for many, since that is what we want to push and what is also immediately useful to people.

In both Text and Talk the width of the text is limited, so I’m a bit less worried about catching people when scrolling. But we could also think about another way of fixing this: Instead of a small vs interactive preview, having a read-only vs interactive preview. Read-only meaning the preview could be overlaid with e.g. a div which prevents interactivity, and an "Edit" button on e.g. the bottom right which removes it and makes stuff fully editable.

This would still make the feature very visible but not hijack the scroll. However there we should still use the logic to only show it in this "read-only" mode in apps like Talk where things are not mainly editable.

What do you think @juliushaertl?

@jancborchardt
Copy link
Contributor

While scrolling down this unrelated issue and its code embeddings, I noticed that the scroll was not hijacked by the code embeddings even if exactly over them: nextcloud/server#43393

Is this some ~100 ms timeout GitHub does in the frontend, or native browser functionality? Maybe this is a way to fix the issue.

@jancborchardt
Copy link
Contributor

Tested elsewhere as well and the scrolling is not hijacked either on e.g. https://stackoverflow.com/questions/73566300/how-can-i-make-contents-within-a-container-scrollable

There is a small timeout and you have to move the mouse again within the scroll container to scroll inside. So I’d say we should go for full interactive embeds?

@juliushaertl
Copy link
Contributor Author

There is a small timeout and you have to move the mouse again within the scroll container to scroll inside. So I’d say we should go for full interactive embeds?

Sorry I haven't mentioned before but there are two other parts where I think having this opt-in option is valuable.

  • Performance: Depending on how complex interactive widgets get they might have quite a large impact on performance loading everything initially, especially when scrolling through a chat message list. By toggling this option we can be sure we only load a reasonable amount of javascript and API data for those on initial load and then fetch anything else on demand
  • Small viewports: This might be especially the case for the talk sidebar in the chat where i think having a simplified preview is a good default. Given the limited space this would otherwise clutter the chat quite a lot I think

@juliushaertl
Copy link
Contributor Author

I've spend quite some time now trying to figure out why this PR no longer works with text, and it seems that nextcloud/text#5402 is somehow causing reactivity of the vueuse useElementVisibility to break. The watcher then never triggers.

Interestingly commenting out the setViewerVue call makes things work while removing the usages of getViewerVue has no effect. I also tried using the viewer vue instance in https://github.com/nextcloud/text/blob/main/src/nodes/Paragraph.js#L8.

@ShGKme As the vue expert around, do you have any idea what is going wrong there? We're just using the component in https://github.com/nextcloud/text/blob/main/src/nodes/ParagraphView.vue#L26-L30

@juliushaertl
Copy link
Contributor Author

Managed to make it work by using a intersection observer directly for now

@juliushaertl juliushaertl marked this pull request as ready for review February 29, 2024 11:46
@juliushaertl juliushaertl added enhancement New feature or request 3. to review Waiting for reviews labels Feb 29, 2024
@ShGKme ShGKme added this to the 8.9.0 milestone Feb 29, 2024
@ShGKme
Copy link
Contributor

ShGKme commented Mar 1, 2024

I've spend quite some time now trying to figure out why this PR no longer works with text, and it seems that nextcloud/text#5402 is somehow causing reactivity of the vueuse useElementVisibility to break. The watcher then never triggers.

Interestingly commenting out the setViewerVue call makes things work while removing the usages of getViewerVue has no effect. I also tried using the viewer vue instance in https://github.com/nextcloud/text/blob/main/src/nodes/Paragraph.js#L8.

@ShGKme As the vue expert around, do you have any idea what is going wrong there? We're just using the component in https://github.com/nextcloud/text/blob/main/src/nodes/ParagraphView.vue#L26-L30

Hard to say without reproduction...

@juliushaertl juliushaertl requested a review from elzody March 1, 2024 13:19
@juliushaertl
Copy link
Contributor Author

Yeah sorry, should have provided that. I'll see if I can come up with a simple reproducer when things are a bit less busy, but for now the workaround seems fine.

…azy once in view

Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Copy link
Contributor

@max-nextcloud max-nextcloud left a comment

Choose a reason for hiding this comment

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

Just went over the code once and did not spot anything obvious. Please let me know if you'd like a more detailed review.

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.

Same as max-nextcloud

@skjnldsv skjnldsv merged commit 3b81497 into master Mar 5, 2024
19 checks passed
@skjnldsv skjnldsv deleted the enh/interactive-toggle-lazy branch March 5, 2024 14:51
@ShGKme
Copy link
Contributor

ShGKme commented Mar 8, 2024

/backport to next

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 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants