Conversation
|
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch.
|
hannessolo
left a comment
hannessolo
left a comment
There was a problem hiding this comment.
Since you added icons to the code in this PR, I think it would be a good opportunity to switch the icon loading methods - load them directly from da.live/img/icons/... instead.
| @@ -1,2 +1 @@ | |||
| npx lint-staged | |||
| npm test | |||
There was a problem hiding this comment.
Looks like it was unintentional. Added it back. @auniverseaway, hope you are good with that.
| const ICONS = [ | ||
| `${nxBase}/blocks/loc/img/Smock_Checkmark_18_N.svg`, | ||
| `${nxBase}/blocks/loc/img/Smock_ChevronRight_18_N.svg`, | ||
| `${nx}/public/icons/Smock_Checkmark_18_N.svg`, |
There was a problem hiding this comment.
Optional: could update this to the new way of loading icons ( directly from da.live)
|
|
||
| const ICONS = [ | ||
| `${nxBase}/blocks/loc/img/Smock_ChevronRight_18_N.svg`, | ||
| `${nx}/public/icons/Smock_ChevronRight_18_N.svg`, |
| return { org, repo, pathname }; | ||
| }; | ||
|
|
||
| function replaceHtml(text, fromOrg, fromRepo, options = {}) { |
There was a problem hiding this comment.
Can you double-check whether we already have a similar function in nx2, and if yes move to a shared utils location?
There was a problem hiding this comment.
I didn't find it in nx2. I believe that the project/ folder was used for legacy Rollout and Sync. So, it didn't make sense to move it to common utils. Let me know if you still want to move it.
| @@ -0,0 +1,11 @@ | |||
| button, | |||
There was a problem hiding this comment.
Could we potentially use the styles from nx2/styles/form.css instead?
There was a problem hiding this comment.
I don't think it makes sense to import all of the form styles just to reset the buttons. What do you think?
| const ICONS = [ | ||
| `${nx}/blocks/loc/img/Smock_Close_18_N.svg`, | ||
| `${nx}/blocks/loc/img/Smock_Add_18_N.svg`, | ||
| `${nx}/public/icons/Smock_Close_18_N.svg`, |
There was a problem hiding this comment.
See comments about icons
| return strings.get(key) ?? key; | ||
| }; | ||
|
|
||
| export const loadStyle = (() => { |
There was a problem hiding this comment.
Is this move intentional?
There was a problem hiding this comment.
@auniverseaway, not sure if this was intentional on your part.
| @@ -0,0 +1,14 @@ | |||
| const config = await (async () => { | |||
There was a problem hiding this comment.
What does this do? Is it just for testing or do we need it? The goal is for evertyhing to be nx2 only.
There was a problem hiding this comment.
@auniverseaway , not sure if we need this at all. I updated the utils.js (f980af1#diff-e3c0e4a8446171228c63c5f2441f0fdb889f13af246617ff25d6c3e4cd8bbd85R216) to load from a relative path instead of using nx variable. The only other place this was used is nx/blocks/loc/project/views/translate.js which is the legacy implementation.
4 participants
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 #532
Test URLs: