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

SITES-16562 - [Xwalk] Open Universal Editor from Franklin Sidekick #17

Merged
merged 4 commits into from
Apr 18, 2024

Conversation

jckautzmann
Copy link
Collaborator

@jckautzmann jckautzmann commented Apr 16, 2024

  • adds a customized Edit button to the Sidekick to open the page with the Universal Editor
  • hides the default buttons of the Sidekick

Fix SITES-16562

Test URLs:

Example:
Screenshot 2024-04-17 at 18 02 28

Copy link

aem-code-sync bot commented Apr 16, 2024

Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
In case there are problems, just click a checkbox below to rerun the respective action.

  • Re-run PSI checks
  • Re-sync branch
Commits

@jckautzmann
Copy link
Collaborator Author

The aem-psi-check fails with the message:

Pull request description must include at least one preview url to the test project.
Please update the description.

although the description contains test URLs.

@jckautzmann jckautzmann requested a review from buuhuu April 17, 2024 16:17
// hide the default buttons
const container = await getElement(sk, '.plugin-container');
container.style.visibility = 'hidden';
for (let i = 0; i < ['edit', 'reload', 'publish', 'delete', 'unpublish'].length; i += 1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

use ['edit', 'reload', 'publish', 'delete', 'unpublish'].forEach(async (action) => { ... }) instead

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was giving a linting error. That's why I used the for loop.

Copy link
Collaborator

Choose a reason for hiding this comment

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

but then at least define array as a variable and don't duplicate it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

addressed in 5c102d6

@@ -142,6 +143,7 @@ function loadDelayed() {
// eslint-disable-next-line import/no-cycle
window.setTimeout(() => import('./delayed.js'), 3000);
// load anything that can be postponed to the latest here
initSidekick();
Copy link
Collaborator

Choose a reason for hiding this comment

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

better do an import('./sidekick.js').then(({ initSidekick }) => initSidekick()); here

that defers loading the script until after loadLazy finished. with the static import at the top scripts.js only executes with the script loaded and hence we impact LCP.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

addressed in 8fd5333

scripts/sidekick.js Outdated Show resolved Hide resolved
scripts/sidekick.js Outdated Show resolved Hide resolved
scripts/sidekick.js Outdated Show resolved Hide resolved
@buuhuu buuhuu merged commit 4ed2b52 into main Apr 18, 2024
2 of 3 checks passed
omprakashgupta1995 referenced this pull request in WWWPiramalFinanceCOM/piramalfinance Jul 2, 2024
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