FORMS-26630: Add terms and conditions component - #179
Conversation
armaang1729
left a comment
armaang1729
left a comment
There was a problem hiding this comment.
Overall: good addition of the TnC component. A few bugs and one security issue need addressing before merge.
| const { id, label, required, enumNames, enum: enums, value, name, readOnly, visible, enabled, appliedCssClassNames, valid } = props; | ||
| const options = enumNames && enumNames.length ? enumNames : enums || []; | ||
| const orientation = props.layout?.orientation.toUpperCase(); | ||
| const isToggleableLink = props[':type'].includes('toggleablelink'); |
There was a problem hiding this comment.
Bug (crash risk): props[':type'] is optional — .includes() will throw if it's undefined (e.g. plain checkbox-group without :type set).
const isToggleableLink = props[':type']?.includes('toggleablelink') ?? false;| /> | ||
| {richTextString(item)} | ||
| {isToggleableLink ? | ||
| <a className='cmp-adaptiveform-checkboxgroup__links' |
There was a problem hiding this comment.
Security: target='_blank' without rel opens reverse tabnapping. Add rel="noopener noreferrer".
<a
className='cmp-adaptiveform-checkboxgroup__links'
target='_blank'
rel='noopener noreferrer'
href={enums![index]}
title={item || ''}
onClick={() => linkClickHandler(index)}
>| const closeIconLabel = i18n.formatMessage({ id: 'termsAndConditions.closeButton.ariaLabel', defaultMessage: 'Close terms and conditions document' }); | ||
|
|
||
| useEffect(() => { | ||
| const textItem = getElementByFieldType('plain-text'); |
There was a problem hiding this comment.
Bug (ReferenceError): getElementByFieldType is a const arrow function declared at line 64 — const is NOT hoisted. Calling it here at line 38 (inside useEffect) will throw ReferenceError: Cannot access 'getElementByFieldType' before initialization at runtime.
Move both getElementByFieldType and getElementsByFieldType declarations above this useEffect.
There was a problem hiding this comment.
This does not break, as function is being called from inside useEffect, but still will move this upwards for readability
| if(itemInForm) { | ||
| form.getElement(checkbox.id).enabled = true; | ||
| } | ||
| observer.unobserve(node); |
There was a problem hiding this comment.
Bug: observer.unobserve(node) is inside the forEach loop — it fires on every checkbox iteration, not just once after all are processed. If checkboxList has more than one entry, subsequent iterations still run but the observer is already detached. Move unobserve outside the forEach:
checkboxList.forEach((checkbox: any) => {
if (checkbox) {
const itemInForm = form.getElement(checkbox.id);
if (itemInForm) {
form.getElement(checkbox.id).enabled = true;
}
}
});
observer.unobserve(node);| <button type="button" className='cmp-adaptiveform-termsandcondition__close-button' | ||
| aria-label={closeIconLabel} | ||
| onClick={()=>toggleModal(false)}>X</button> | ||
|
|
There was a problem hiding this comment.
Close button text is a literal X. Visually poor and not consistent with other AEM components. Use × or an SVG icon. The aria-label is correct so accessibility is fine, but the visual presentation needs improvement.
There was a problem hiding this comment.
This markup is kept same as in core components, let me know if we should update in this PR or do it after updating in CC
| items.map((item: any, index) => { | ||
| // text or link render below | ||
| const classSuffix = item.fieldType === 'plain-text' ? 'text' : item.fieldType === 'checkbox-group' ? 'link' : null; | ||
| { |
There was a problem hiding this comment.
Nit: Extra {} block inside the .map() body does nothing — it creates a new block scope but doesn't change behaviour. Remove it:
return (classSuffix && (
<div key={item.id} className={`cmp-adaptiveform-termsandcondition__${classSuffix}`}>
...
</div>
));| <div | ||
| className='cmp-adaptiveform-termsandcondition__approvalcheckbox' | ||
| onClick={handleApprovalCheckboxClick}> | ||
| {approvalCheckboxItem && getChild(approvalCheckboxItem, 1, mappings)} |
There was a problem hiding this comment.
Bug: getChild(approvalCheckboxItem, 1, mappings) hardcodes index 1. The approval checkbox may not always be at position 1 in items. Use the actual index:
const approvalCheckboxIndex = items.findIndex(item => item.fieldType === 'checkbox');
// then:
getChild(approvalCheckboxItem, approvalCheckboxIndex, mappings)There was a problem hiding this comment.
This is not under a loop, and was for single element found after using find. GetChild makes key using this element's id and index. I will still implement fix for clarity
| @@ -0,0 +1,219 @@ | |||
| /* | |||
There was a problem hiding this comment.
Wrong license header. This file has the closed-source Adobe beta license. All other test files in this repo use Apache 2.0. Replace with the standard Apache 2.0 header matching the rest of the codebase (e.g. see CheckBoxGroup.test.tsx).
There was a problem hiding this comment.
This text is same as other files, could be false positive as i updated year to 2026
armaang1729
left a comment
armaang1729
left a comment
There was a problem hiding this comment.
Please see the review comments !
|
Will update other suggestions and share implementation screenshots of all modes |
2 participants
Description
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: