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 the Monaco editor Fields support preview #10777

Merged
merged 8 commits into from
Feb 10, 2022

Conversation

hyzx86
Copy link
Contributor

@hyzx86 hyzx86 commented Nov 29, 2021

Modify HtmlField and TextField to use Ctrl-S to trigger previews when using the Monaco editor

动画

@sebastienros
Copy link
Member

Why not on each key stroke like the other editors?

@hyzx86
Copy link
Contributor Author

hyzx86 commented Dec 3, 2021

I don't know much about Monaco, but I see that this code is only going to trigger the get value action when the submit is clicked, so I think this is a performance optimization.

 window.addEventListener("submit", function () {
                textArea.value = editor.getValue();
});

Of course I could also add custom switch options for the Monaco editor, for example, the default is to trigger the update by each key, and Ctrl+S only after the switch is turned on , I think this would be a better solution.

@deanmarcussen
Copy link
Member

Noone will ever know to enable it with Ctrl+S

Can you try with just keystrokes and if the performance is fine, I think that will be ok

@MichaelPetrinolis
Copy link
Contributor

If there are still any performance issues, adding a throttle on keystrokes might solve them.

change previewButton  use named window
@hyzx86
Copy link
Contributor Author

hyzx86 commented Dec 3, 2021

updated , and change the preview fonction, use a named window to avoid always open new window

preview2

@@ -10,7 +10,7 @@

@if (hasPreviewPermission)
{
<a id="previewButton" target="_blank" class="btn btn-info" href="@Url.Action("Index", "Preview", new { area = "OrchardCore.ContentPreview", id = previewId })">@T["Preview"]</a>
<a id="previewButton" target="previewWindow" class="btn btn-info" href="@Url.Action("Index", "Preview", new { area = "OrchardCore.ContentPreview", id = previewId })">@T["Preview"]</a>
Copy link
Member

Choose a reason for hiding this comment

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

@Skrypt do you agree with this change? I think I do (but equally if I have multiple Orchard instances running (dev and live), it will confuse me when it keeps swapping windows around on me?

Other changes look good to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure, to be honest. Could we not have a unique identifier for this target instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

add the host name as perfix?

Copy link
Member

@deanmarcussen deanmarcussen Dec 31, 2021

Choose a reason for hiding this comment

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

I would prefer removing it, and opending a seperate issue so we can have a proper review of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@deanmarcussen , target="previewWindow" removed

@hyzx86 hyzx86 requested review from deanmarcussen and Skrypt January 7, 2022 15:11
@@ -46,7 +46,7 @@

var editor = monaco.editor.create(document.getElementById('@Html.IdFor(x => x.Html)_editor'), settings);
var textArea = document.getElementById('@Html.IdFor(x => x.Html)');

editor.getModel().onDidChangeContent((event) => {
Copy link
Member

Choose a reason for hiding this comment

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

How come I see the exact same code that you're adding up on L50?

i.e. don't we already do this?

           //for preview
            if(document.getElementById("previewButton")){
                editor.getModel().onDidChangeContent((event) => {
                    textArea.value = editor.getValue();
                    $(document).trigger('contentpreview:render');
                });
            }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes , you are right ..
I didn't notice the codes 。

@@ -49,6 +49,13 @@
window.addEventListener("submit", function () {
textArea.value = editor.getValue();
});
//for preview
if(document.getElementById("previewButton")){
Copy link
Member

Choose a reason for hiding this comment

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

Don't think we need this check, we don't use it anywhere else.

Can you remove for consistency?

Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done😊

textArea.value = editor.getValue();
$(document).trigger('contentpreview:render');
});

Copy link
Member

Choose a reason for hiding this comment

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

Remove the extra line, you could add it before line 52

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. Please format the code appropriately.

@sebastienros sebastienros merged commit 169d324 into OrchardCMS:main Feb 10, 2022
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.

6 participants