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: Make edit mode in interactive widgets opt-in #3619

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

juliushaertl
Copy link
Member

@juliushaertl juliushaertl commented Apr 24, 2024

Fix #3540

A copy of nextcloud/text#5579 in most parts

Screenshot 2024-04-24 at 08 47 11

@juliushaertl juliushaertl added bug Something isn't working 3. to review Ready to be reviewed labels Apr 24, 2024
@juliushaertl
Copy link
Member Author

/backport to stable29

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.

Code looks good. Added two minor comments.

@@ -83,11 +83,18 @@
:style="{visibility: showIframe ? 'visible' : 'hidden' }"
:src="iframeSrc" />

<NcButton v-if="isEmbedded && !hasToggledInteractiveEmbedding" class="toggle-interactive" @click="toggleEdit">
{{ t('text', 'Edit') }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{{ t('text', 'Edit') }}
{{ t('richdocuments', 'Edit') }}

Copy link
Member Author

Choose a reason for hiding this comment

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

🙈

Comment on lines 479 to 482
toggleEdit() {
this.hasToggledInteractiveEmbedding = true
},

Copy link
Contributor

Choose a reason for hiding this comment

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

Minor naming nitpicks:

  • This does not really toggle. It only sets to true.
  • the names do not match well - Edit vs. InteractiveEmbedding.

How about this?

Suggested change
toggleEdit() {
this.hasToggledInteractiveEmbedding = true
},
enableEditing() {
this.hasEditingEnabled = true
},

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, used hasWidgetEditingEnabled now to be more explicit about where its used.

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

Cypress fails due to the new 24.04 release of Collabora with changed selectors, will address separately

@juliushaertl juliushaertl merged commit c4142e1 into main Apr 24, 2024
42 of 44 checks passed
@juliushaertl juliushaertl deleted the fix/interactive-read-only branch April 24, 2024 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Ready to be reviewed bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Viewer should load read only mode when interactive widget is requested
2 participants