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

Make collabora save and exit actions more intuitive #12 #16

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

oanalavinia
Copy link
Collaborator

  • add 2 new buttons in the editor menu: save and exit + close editor and use the postMesage API to handle the events

The buttons in the menu:
image

The popup displayed when the user wants to close the editor and there are unsaved changes:
image

* add 2 new buttons in the editor menu: save and exit + close editor and use the postMesage API to handle the events
@oanalavinia
Copy link
Collaborator Author

oanalavinia commented Mar 30, 2023

There are some things that should be considered for this UI.

  1. Even if save and exit and close editor are near the save button, there is also a File->Save where these are not present and it might lead to confusion.
    image
  2. It would have been helpful to be able to define these buttons using also a label, but the API does not have this option
  3. Usually the close editor action will be placed on the right up corner (at least for Windows users), so the current position it's not very intuitive. However, the insertBefore API seems to not work as expected, so we cannot actually change the position, but maybe we could at least reverse the order of the buttons, to have close at the margin. To be checked

Copy link
Contributor

@mflorea mflorea left a comment

Choose a reason for hiding this comment

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

Looks good.

$escapetool.xml($fileDoc.displayTitle)
</a>
#macro (unsavedChangesModal)
<div class="modal fade" id="editUnsavedChanges" tabindex="-1" role="dialog" aria-hidden="true"
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
<div class="modal fade" id="editUnsavedChanges" tabindex="-1" role="dialog" aria-hidden="true"
<div class="modal fade" id="collaboraUnsavedChanges" tabindex="-1" role="dialog" aria-hidden="true"

<input type="submit" class="btn btn-danger"
value="$escapetool.xml($services.localization.render('collabora.editor.unsaved.submit'))">
<input type="button" class="btn btn-default" data-dismiss="modal"
value="$escapetool.xml($services.localization.render('collabora.editor.unsaved.close'))">
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to also have the option to save before closing the editor?

<a href="${xwiki.getURL('Main.WebHome')}"><img src="$xwiki.getSkinFile('logo.svg')"></a>
</div>
<div>
<strong>$currentAction</strong>
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you do the escaping below, when setting the value, but I find it more clear to perform the escaping in the place where the value is used (because the escaping depends on the context where the value is used). specially since you use the value only once, here.

'SendTime': Date.now(),
'Values': {
'id': 'saveAndExitEditor',
'imgurl': window.location.origin + $("#collaboraConfig").data('saveExitIcon'),
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
'imgurl': window.location.origin + $("#collaboraConfig").data('saveExitIcon'),
'imgurl': new URL($("#collaboraConfig").data('saveExitIcon'), window.location.origin),

'SendTime': Date.now(),
'Values': {
'id': 'closeEditor',
'imgurl': window.location.origin + $("#collaboraConfig").data('closeIcon'),
Copy link
Contributor

Choose a reason for hiding this comment

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

See above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants