-
Notifications
You must be signed in to change notification settings - Fork 1
WIP: Issue/99 Update UE instrumentation to use data-aue-component #109
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
WIP: Issue/99 Update UE instrumentation to use data-aue-component #109
Conversation
src/ue/attributes.js
Outdated
| 'data-aue-label': 'Page', | ||
| 'data-aue-type': 'component', | ||
| 'data-aue-model': 'page-metadata', | ||
| 'data-aue-component': 'page-metadata', |
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.
here I'm not sure if we should call it page instead of page-metadata. Also, will this be within the component-definition? Can we create a page-metadata component?
If not, it would make sense to keep this one with data-aue-model
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 used the page-metadata model for page wide properties. So far page-metadata was not in the component definition, only in models definition.
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.
Then let's keep it page-metadata. As it is neither supposed to be added/removed/moved/copied, I would keep it as it was with data-aue-model
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.
reverted to data-aue-model
src/ue/attributes.js
Outdated
| 'data-aue-type': 'container', | ||
| 'data-aue-label': 'Main Content', | ||
| 'data-aue-filter': 'main', | ||
| 'data-aue-component': 'main', |
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.
Same as above main is unique and usually you can not add a new main component.
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.
Correct, projects can not do anything on main we only need it for the filter as main has n sections. Should we keep the filter here?
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'd say so, no data-aue-component, just the filter as it was
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.
reverted to filter
src/ue/attributes.js
Outdated
| */ | ||
| function addBlockFieldAttributes(ueConfig, block) { | ||
| const blockName = block.properties['data-aue-model']; | ||
| const blockName = block.properties['data-aue-component']; |
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.
Should we keep reading data-aue-model as fallback for projects which have not adjusted the instrumentation files. With the assumption the model name == component name? @maximilianvoss WDYT?
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.
How many customers would we potentially break?
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.
3 live sites I'm aware of (but we have good contact to ACS) and x number of commerce team demos.
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.
added fallback
src/ue/attributes.js
Outdated
| 'data-aue-label': 'Page', | ||
| 'data-aue-type': 'component', | ||
| 'data-aue-model': 'page-metadata', | ||
| 'data-aue-component': 'page-metadata', |
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 used the page-metadata model for page wide properties. So far page-metadata was not in the component definition, only in models definition.
src/ue/attributes.js
Outdated
| 'data-aue-type': 'container', | ||
| 'data-aue-label': 'Main Content', | ||
| 'data-aue-filter': 'main', | ||
| 'data-aue-component': 'main', |
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.
Correct, projects can not do anything on main we only need it for the filter as main has n sections. Should we keep the filter here?
|
I marked the PR as WIP. As this will be breaking and while we can deploy this to STAGE we should not get this on the main branch as it will break projects. Lets collect all breaking changes in a "v1" branch. |
|
@mhaack by
do you mean this branch shoul dbe renamed, or we just consider this branch a "v1"? |
No lets leave it, just don't merge to main. We have no automatic stage deployment for the worker. I was thinking we add a stage branch which we merge into and use that for stage deployments. Too be discussed. |
wrangler.toml
Outdated
|
|
||
| [env.dev] | ||
| vars = { ENVIRONMENT = "dev", UE_HOST = "localhost:4712", UE_SERVICE = "https://localhost:8000", DA_ADMIN = "https://admin.da.live" } | ||
| vars = { ENVIRONMENT = "dev", UE_HOST = "localhost:4712", UE_SERVICE = "https://localhost.corp.adobe.com:8000", DA_ADMIN = "https://admin.da.live" } |
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.
will require that you are on VPN the whole time. Didn't it work with localhost?
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.
sorry I meant to move it to another PR for the configuration template, I'll remove it from here. That doesn't require VPN, it's ought to be registered at /etc/hosts to work but the benefit is we have a certificate for that. But that's another story.
Description
Adjusting the code to use data-aue-component property instead of data-aue-model and/or data-aue-filter and da-aue-behaviour.
The page configuration PR is at aemsites/da-block-collection#21
Related Issue
#99
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: