UX: Show loading spinner while loading dependencies for ace-editor #26099
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
OsamaSayegh
approved these changes
Mar 9, 2024
CvX
reviewed
Mar 10, 2024
Comment on lines
59
to
61
| await waitUntil(() => { | ||
| return document.querySelectorAll(".ace_line")[0]; | ||
| }); |
There was a problem hiding this comment.
🏌️
Suggested change
| await waitUntil(() => { | |
| return document.querySelectorAll(".ace_line")[0]; | |
| }); | |
| await waitUntil(() => document.querySelector(".ace_line")); |
Comment on lines
63
to
67
| const indexOf = document | ||
| .querySelectorAll(".ace_line")[0] | ||
| .innerHTML.indexOf("["); | ||
|
|
||
| assert.ok(indexOf >= 0); |
There was a problem hiding this comment.
Suggested change
| const indexOf = document | |
| .querySelectorAll(".ace_line")[0] | |
| .innerHTML.indexOf("["); | |
| assert.ok(indexOf >= 0); | |
| assert.dom(".ace_line").hasText("["); |
| @@ -166,14 +178,17 @@ export default class AceEditor extends Component { | |||
| } | |||
|
|
|||
| @bind | |||
| setAceTheme() { | |||
| aceTheme() { | |||
Comment on lines
1
to
5
| {{loading-spinner size="small"}} | ||
| {{/if}} | ||
|
|
||
| <div class="ace hidden">{{this.content}}</div> |
There was a problem hiding this comment.
Can we not render rather than setting the hidden class?
Suggested change
| {{#if this.isLoading}} | |
| {{loading-spinner size="small"}} | |
| {{/if}} | |
| <div class="ace hidden">{{this.content}}</div> | |
| {{#if this.isLoading}} | |
| {{loading-spinner size="small"}} | |
| {{else}} | |
| <div class="ace">{{this.content}}</div> | |
| {{/if}} |
There was a problem hiding this comment.
It turns out I can. Initially, I was hiding it because the theme file took time to load and I wanted to avoid the flash. Now that I am loading the theme file upfront, I don't need to hide the div anymore.
| window.ace.require(["ace/ace"], (loadedAce) => { | ||
| loadedAce.config.set("loadWorkerFromBlob", false); | ||
| loadedAce.config.set("workerPath", getURL("/javascripts/ace")); // Do not use CDN for workers | ||
| loadScript(`/javascripts/ace/theme-${this.aceTheme()}.js`).then(() => { |
There was a problem hiding this comment.
We didn't have to load the theme separately before?
There was a problem hiding this comment.
Yup it was loaded on demand previously which resulted in the "flash". You can see it clearly in the before screenshot I posted. The solution stated in ajaxorg/ace#3286 is to load the theme file first.
tgxworld
force-pushed
the
fix_ace_editor
branch
2 times, most recently
from
663d819
to
437f3f4
Compare
Why this change? On a slow network, using the `AceEditor` component will result in a blob of text being shown first before being swapped out with the `ace.js` editor after it has completed loading. There is also a problem when setting the theme for the editor which would result in a "flash" as reported in ajaxorg/ace#3286. To avoid this, we need to load the theme js file before displaying the editor. What does this change do? 1. Adds a loading spinner and set the `div.ace` with a `.hidden` class. 2. Once all the relevant scripts and initialization is done, we will then remove the loading spinner and remove `div.ace`.
tgxworld
force-pushed
the
fix_ace_editor
branch
from
437f3f4
to
fb7ef84
Compare
|
Thank you for reviewing @OsamaSayegh and @CvX 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why this change?
On a slow network, using the
AceEditorcomponent will result in a blobof text being shown first before being swapped out with the
ace.jseditor after it has completed loading.
There is also a problem when setting the theme for the editor which
would result in a "flash" as reported in
ajaxorg/ace#3286. To avoid this, we need to
load the theme js file before displaying the editor.
What does this change do?
div.acewith a.hiddenclass.then remove the loading spinner and remove
div.ace.Reviewer notes
There are no tests here but I discovered this problem when trying to write a system tests and interacting with the ace editor. Because of the previous flashing behaviour, any interactions with the ace editor is unreliable and leads to flaky system tests.
Recording
Before
After