-
Notifications
You must be signed in to change notification settings - Fork 4
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
[READY] Feature virtual dom transform #46
Conversation
Merge main branch
Merge main repo
[ready] Feature virtual transforms
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
|
@@ -354,7 +358,16 @@ export function mergeUniqueArrays(...arrays) { | |||
} | |||
|
|||
export function getBaseUrl() { |
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 have removed the usage of basepath in this PR #29
Should this be brought back?
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.
Yes, since on the other SharePoint configs are different, making things break when we have fixed paths like before
|
||
export default class ComponentBase extends HTMLElement { | ||
// All supported data attributes must be added to observedAttributes | ||
// The order of observedAttributes is the order in which the values from config are added. | ||
static observedAttributes = []; | ||
|
||
dataAttributesKeys = []; |
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.
This should be moved in the setDefaults() method for consistency
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.
Actually, all those should be class properties.
To make sure they are defined before the constructor, for instance, if we need to override a property from HTMLElement that need to be set as normal OOP class prop before super is called
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.
In my tests in the begining that worked as it was before, we could change from an element directly any of these properties.
And for config
and nestedComponentsConfig
which we are extending in components keeping the defaults from base, the extension failed if these were class fields.
1 - Virtual dom transformations by reducer
2 - Removal of component loader
3 - Updates on Component Base
4 - Updates on the grid in favor of CSS variables and one-level grid
Test URLs: