few properties were not getting updated via rules on form initialization #1274
Conversation
70676c1 to
ced74c9
Compare
Lighthouse scores (mobile)
|
Lighthouse scores (desktop)
|
Accessibility Violations Found
|
| @@ -111,6 +111,11 @@ Cypress.on('uncaught:exception', (err, runnable) => { | |||
| return false; | |||
| } | |||
|
|
|||
| // this error can come if properties are set via rule editor on form initialization (Since field are yet not initialized completely) | |||
| if (err.message.includes('some error occurred while triggering event on guideBridge')) { | |||
There was a problem hiding this comment.
The test case should fail, why is this part of whitelist ? Please remove this
| const formContainerPath = this.formContainer.getPath(); | ||
| window.guideBridge.trigger(eventType, eventPayload, formContainerPath); | ||
| } catch (e) { | ||
| console.log('some error occurred while triggering event on guideBridge', e); |
There was a problem hiding this comment.
Why did you add a try catch here, ideally the code should break. Can you describe the use-case
f333ae0 to
8e15491
Compare
| @@ -263,13 +263,13 @@ describe("setFocus on the first invalid field on submission", () => { | |||
| }); | |||
|
|
|||
| it("check if invalid field gets focus on submit button click", () => { | |||
| cy.get('button').should('be.visible').click().then(() => { | |||
| cy.get('button').last().should('be.visible').click().then(() => { | |||
There was a problem hiding this comment.
Please don't change anything in an existing test case, unless there is backward incompatible change
There was a problem hiding this comment.
Previously we did not include the questionMark div if long description was not set, but since the setProperty can be called at runtime and even if long description was not set we need to set it in runtime, then we need to have this div. As per this change https://github.com/adobe/aem-core-forms-components/pull/1274/files#:~:text=%3Cbutton%20class%3D%22%24%7BbemBlock%7D__questionmark%22%20style%3D%22%24%7BlongDescription%20%3F%20%27%27%20%3A%20%27display%3Anone%3B%27%7D%20title%3D%22Help%20text%22%3E
Now in the test cases the selectors were wrong as it was assuming that there would only be one button in the form, so I had to update the selectors.
Not sure if this would be backward incompatible though
There was a problem hiding this comment.
But this would pollute the HTML with unncessary markup for each field, thus can impact HTML parsing even for use-cases where description is not set at runtime, which is incorrect.
Ideally, at runtime, we should generate the markup if description is set at runtime.
8e15491 to
eb407cd
Compare
| this.description.querySelector("p").innerHTML = description; | ||
| const questionMarkDiv = this.getQuestionMarkDiv(); | ||
| if (questionMarkDiv) { | ||
| questionMarkDiv.style.display = "block"; |
There was a problem hiding this comment.
We should not be updating styles via code, this might override the theme, and might conflict. What is the use-case to override style
There was a problem hiding this comment.
The requirement is that if long description is not set during authoring then the questionMark div and description div should be present in DOM but hidden. And if by any rule the description is set in runtime, then they should be visible. I guess we could do that by adding 'data-cmp-visible' attribute instead
There was a problem hiding this comment.
yes, please check this comment on the overall design, #1274 (comment)
| @@ -1,4 +1,4 @@ | |||
| <template data-sly-template.questionMark="${@ componentId, longDescription, bemBlock}"> | |||
| <button class="${bemBlock}__questionmark" data-sly-test="${longDescription}" title="Help text"> | |||
| <button class="${bemBlock}__questionmark" style="${longDescription ? '' : 'display:none;'} title="Help text"> | |||
There was a problem hiding this comment.
styles need to come via theme, code should not add inline style, it would override other styles provided by customer. This looks incorrect to me
| <div class="${bemBlock}__longdescription" id="${componentId}__longdescription" style="${wcmmode.edit && longDescription ? '' : 'display:none;'} aria-live="polite"> |
There was a problem hiding this comment.
Why do we need inline style
|
Also, do we have a customer who has asked for this change ? Can you link the JIRA in the PR |
67c2dab to
2146629
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #1274 +/- ##
=========================================
Coverage 81.67% 81.67%
Complexity 853 853
=========================================
Files 97 97
Lines 2259 2259
Branches 304 304
=========================================
Hits 1845 1845
Misses 255 255
Partials 159 159 ☔ View full report in Codecov by Sentry. |
Lighthouse scores (mobile)
|
Lighthouse scores (desktop)
|
Accessibility Violations Found
|
2 similar comments
Accessibility Violations Found
|
Accessibility Violations Found
|
052c13a to
c214205
Compare
Lighthouse scores (desktop)
|
Lighthouse scores (mobile)
|
Accessibility Violations Found
|
2 similar comments
Accessibility Violations Found
|
Accessibility Violations Found
|
26fbb13 to
765666f
Compare
Lighthouse scores (desktop)
|
Lighthouse scores (mobile)
|
Accessibility Violations Found
|
2 similar comments
Accessibility Violations Found
|
Accessibility Violations Found
|
| } else { | ||
| this.#addDescriptionInRuntime(); | ||
| } | ||
| this.getDescription().querySelector("p").innerHTML = descriptionText; |
There was a problem hiding this comment.
You would need to sanitize this HTML, we already do this for enumNames,
window.DOMPurify ? window.DOMPurify.sanitize(descriptionText) : descriptionText
| * @param {string} descriptionText - The description. | ||
| */ | ||
| updateDescription(descriptionText) { | ||
| if (descriptionText !== undefined) { |
There was a problem hiding this comment.
if (typeof descriptionText !== 'undefined') {
const descriptionElement = this.getDescription();
if (descriptionElement) {
let pElement = descriptionElement.querySelector("p");
if (!pElement) {
// If the description is updated via rule then it might not have <p> tags
pElement = document.createElement('p');
descriptionElement.appendChild(pElement);
}
pElement.textContent = descriptionText;
} else {
this.#addDescriptionInRuntime(descriptionText);
}
}
```
| } | ||
| } | ||
|
|
||
| #addDescriptionInRuntime() { | ||
| console.log('adding description in runtime') |
| #addDescriptionInRuntime() { | ||
| console.log('adding description in runtime') | ||
| // add question mark icon | ||
| const bemClass = Array.from(this.element.classList).filter(bemClass => !bemClass.includes('--'))[0]; |
There was a problem hiding this comment.
lets make the code NPE safe
// add question mark icon
const bemClass = Array.from(this.element.classList).find(bemClass => !bemClass.includes('--'));
const labelContainer = this.element.querySelector(`.${bemClass}__label-container`);
if (labelContainer) {
const qmButton = document.createElement('button');
qmButton.className = `${bemClass}__questionmark`;
qmButton.title = 'Help text';
labelContainer.appendChild(qmButton);
} else {
console.error('Label container not found');
return;
}
// add description div
const descriptionDiv = document.createElement('div');
descriptionDiv.className = `${bemClass}__longdescription`;
descriptionDiv.id = `${this.getId()}__longdescription`;
descriptionDiv.setAttribute('aria-live', 'polite');
descriptionDiv.setAttribute('data-cmp-visible', 'false');
const pElement = document.createElement('p');
if (typeof descriptionText !== 'undefined') {
pElement.textContent = descriptionText;
}
descriptionDiv.appendChild(pElement);
const errorDiv = this.getErrorDiv();
if (errorDiv) {
this.element.insertBefore(descriptionDiv, errorDiv);
} else {
console.error('Error div not found');
return;
}
765666f to
f747f3b
Compare
few properties were not getting updated via rules on form initialization few properties were not getting updated via rules on form initialization few properties were not getting updated via rules on form initialization update properties fix via rule editor
few properties were not getting updated via rules on form initialization
few properties were not getting updated via rules on form initialization
f747f3b to
0f54c90
Compare
Lighthouse scores (desktop)
|
Lighthouse scores (mobile)
|
Accessibility Violations Found
|
2 similar comments
Accessibility Violations Found
|
Accessibility Violations Found
|
…ion (#1274) * This is a combination of 2 commits. few properties were not getting updated via rules on form initialization few properties were not getting updated via rules on form initialization few properties were not getting updated via rules on form initialization update properties fix via rule editor * few properties were not getting updated via rules on form initialization few properties were not getting updated via rules on form initialization * few properties were not getting updated via rules on form initialization few properties were not getting updated via rules on form initialization * update af-core version * few properties were not getting updated via rules on form initialization- refactoring
Description
Some Properties like 'required' 'description' and 'label' were not getting updated when set via rule 'When Form is Initialized'.
Further, the 'description' was never getting updated as if no description was authored initially then the description div was explicitly ignored in the Sightly. As a fix, included the long description div (which is always hidden by default until question mark icon is clicked), and marked the questionMark div as hidden unless we see that the description is present.
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: