-
Notifications
You must be signed in to change notification settings - Fork 59
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
Save As Fragment #1433
base: dev
Are you sure you want to change the base?
Save As Fragment #1433
Conversation
…nd panelcontainer
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #1433 +/- ##
=========================================
Coverage 82.27% 82.27%
Complexity 923 923
=========================================
Files 103 103
Lines 2369 2369
Branches 321 321
=========================================
Hits 1949 1949
Misses 257 257
Partials 163 163 ☔ View full report in Codecov by Sentry. |
Accessibility Violations Found
|
Accessibility Violations Found
|
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.
check comments
@@ -2,4 +2,5 @@ | |||
<jcr:root xmlns:jcr="http://www.jcp.org/jcr/1.0" xmlns:nt="http://www.jcp.org/jcr/nt/1.0" xmlns:cq="http://www.day.com/jcr/cq/1.0" | |||
jcr:primaryType="nt:unstructured" | |||
jcr:title="Fragment" | |||
fragmentPath="" |
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.
Why do we need an empty key 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.
We need a way to distinguish between a panel and a fragment component. So, we changed the component's cq: template to always include fragmentPath
. Otherwise we will have to create new property like fd:viewType to differentiate
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 would prefer fd:viewType rather than fragmentPath, since this is not intuitive.
window.CQ.FormsCoreComponents.Utils = window.CQ.FormsCoreComponents.Utils || {}; | ||
const DataModel = window.CQ.FormsCoreComponents.Utils.DataModel = window.CQ.FormsCoreComponents.Utils.DataModel || {}; | ||
|
||
var FORM_CONTAINER_SELECTOR = "[data-cmp-is='adaptiveFormContainer']"; |
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.
None of this code is going to be changed by core component developers, let's move this code to cq-guides (like we have done for replace / rule editor etc), since this logic is very specific
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, these codes should not require any customization. In that case Data Model code will also required to moved to avoid duplicacy right ?
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, I agree that the data model code shouldn't be included here. We should keep the codebase clean by only including the relevant components that are customizable or useful for developers
</templateselector> | ||
<fragmentselectcontainer | ||
jcr:primaryType="nt:unstructured" | ||
granite:class="coral-Form-fieldwrapper cmp-adaptiveform-saveasfragment__fragmentselector-container" |
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.
Why do you need this class, coral-Form-fieldwrapper
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.
Just to align with style of other field wrapper in dialog . This field is appended on client side while others are rendered at server.
[1]: https://developer.adobe.com/experience-manager/reference-materials/6-5/coral-ui/coralui3/styles.html#coral-Form
.../core/fd/components/form/panelcontainer/v1/panelcontainer/saveasfragment_dialog/.content.xml
Outdated
Show resolved
Hide resolved
...e/fd/components/form/panelcontainer/v1/panelcontainer/clientlibs/editor/js/saveasfragment.js
Outdated
Show resolved
Hide resolved
...e/fd/components/form/panelcontainer/v1/panelcontainer/clientlibs/editor/js/saveasfragment.js
Outdated
Show resolved
Hide resolved
...e/fd/components/form/panelcontainer/v1/panelcontainer/clientlibs/editor/js/saveasfragment.js
Outdated
Show resolved
Hide resolved
...pps/core/fd/components/form/container/v2/container/clientlibs/editorhook/js/fragmentshook.js
Show resolved
Hide resolved
...in/content/jcr_root/apps/forms-components-examples/components/form/fragment/_cq_template.xml
Show resolved
Hide resolved
.../content/jcr_root/apps/core/fd/components/af-commons/v1/clientlibs/editor/utils/datamodel.js
Show resolved
Hide resolved
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.
check comments
.../content/jcr_root/apps/core/fd/components/af-commons/v1/clientlibs/editor/utils/datamodel.js
Show resolved
Hide resolved
...ps/core/fd/components/form/container/v2/container/clientlibs/editorhook/js/componentutils.js
Outdated
Show resolved
Hide resolved
@@ -171,4 +174,34 @@ | |||
|
|||
window.CQ.FormsCoreComponents.editorhooks.allowedCompFieldTypes = []; | |||
|
|||
window.CQ.FormsCoreComponents.editorhooks.getFormContainerProperties = (editablePath) => { | |||
const result = $.ajax({ |
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.
Can we use the new fetch syntax here, rather than using $.ajax
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.
Ajax call is made with async: false
, used in condition 1 to display DOR action in the editable toolbar. Since fetch
is inherently asynchronous, it'll require adding support in the core editor to handle async/await so keeping as it is.
.../content/jcr_root/apps/core/fd/components/af-commons/v1/clientlibs/editor/utils/datamodel.js
Show resolved
Hide resolved
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.
check comments
Description
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: