-
Notifications
You must be signed in to change notification settings - Fork 7
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
401 franklin sidekick library #402
Conversation
Hello, I'm the AEM Code Sync Bot and I will run some test suites that validate the page speed.
|
|
We need to merge the Sidekick library code into the main branch in order to be able to configure the library |
tools/sidekick/library.html
Outdated
<body> | ||
<script | ||
type="module" | ||
src="https://www.hlx.live/tools/sidekick/library/index.js" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should switch it to https://www.aem.live/tools/sidekick/library/index.js
tools/sidekick/library.html
Outdated
<script> | ||
const library = document.createElement('sidekick-library') | ||
library.config = { | ||
base: '/block-library/library.json', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be /tools/sidekick/library.json ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you mean /tools/sidekick/config.json?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tools/sidekick/config.json
Outdated
@@ -1,4 +1,6 @@ | |||
{ | |||
"project": "CN Web", | |||
"host": "main--cn-website--netcentric.hlx.page/", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you sure we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove the project for sure but I am not so sure about removing the host
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where did you get it from and why does it need to point to the preview URL?
|
|
|
|
Please always provide the GitHub issue(s) your PR is for, as well as test URLs where your change can be observed (before and after):
Fix #401
Test URLs: