Skip to content

GAUD-10044: Create FormElementContainerMixin mixin#7016

Open
EdwinACL831 wants to merge 11 commits into
mainfrom
ecollazos/GAUD-10044_nested_form_elements
Open

GAUD-10044: Create FormElementContainerMixin mixin#7016
EdwinACL831 wants to merge 11 commits into
mainfrom
ecollazos/GAUD-10044_nested_form_elements

Conversation

@EdwinACL831
Copy link
Copy Markdown
Contributor

@EdwinACL831 EdwinACL831 commented May 22, 2026

Jira

GAUD-10044

Description

Currently the d2l-form component is able to find nested forms, native form controllers and d2l custom form controllers (through the FormElementMixin)

there is still 1 use case where we just want to group the form controllers logically in a custom web component -- let's call it form form element container. this is because the form is big enough so we want to encapsulate portions of it in logically and semantically groups of controller. In this case, the form has no chance to find those nested form controllers within the form element container.

Solution

To address this we decided to implement the FormElementContainerMixin which accomplishes that.

Edwin Collazos added 2 commits May 22, 2026 14:49
- Handles the use case for nested form container components that are not
  also a form controller
- Adds unit test
- Updates the demo page to demonstrate this new behavior
@EdwinACL831 EdwinACL831 requested a review from a team as a code owner May 22, 2026 19:53
@github-actions
Copy link
Copy Markdown
Contributor

Thanks for the PR! 🎉

We've deployed an automatic preview for this PR - you can see your changes here:

URL https://live.d2l.dev/prs/BrightspaceUI/core/pr-7016/

Note

The build needs to finish before your changes are deployed.
Changes to the PR will automatically update the instance.

Comment thread components/form/demo/custom-form-element-container.js
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change in combination with the changes at components/form/form-helper.js is how now the form is able to "find" these custom form element containers

Comment thread components/form/form-helper.js Outdated
Copy link
Copy Markdown
Member

@dlockhart dlockhart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good -- just a few minor notes about the demo/docs. We'll also want to add form-element-container-mixin.md documentation.

Comment thread components/form/demo/custom-form-element-container.js Outdated
Comment thread components/form/demo/custom-form-element-container.js Outdated
Comment thread components/form/demo/form-demo.js Outdated
Comment thread components/form/test/custom-form-element-container.js Outdated
Comment thread components/form/form-element-container-mixin.js Outdated
Comment thread components/form/form-element-container-mixin.js Outdated
Comment thread components/form/demo/custom-form-element-container.js
Comment thread components/form/demo/form-demo.js Outdated
Comment thread components/form/demo/form-panel-demo.js
Comment thread components/form/form-element-container-mixin.js Outdated
Comment thread components/form/docs/form-element-container-mixin.md Outdated
Comment thread components/form/docs/form-element-container-mixin.md Outdated
Comment thread components/form/docs/form-element-container-mixin.md Outdated
@@ -1,9 +1,11 @@
import './form-element.js';
import '../../inputs/input-text.js';
Copy link
Copy Markdown
Contributor Author

@EdwinACL831 EdwinACL831 May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed because the class defined using defineCE uses it within its render method.

required
class="d2l-input">
</div>
<d2l-input-text label="Middle Name" name="middle-name" minlength="4" maxlength="8"></d2l-input-text>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have an import for input-text.js and input-number.js, which can be removed from form-panel-demo.js.

required
>
<d2l-input-text
id="nested-telephone-input"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: id not required.

Copy link
Copy Markdown
Contributor

@bearfriend bearfriend left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly a bunch of nits

<d2l-input-group>
<div>
<label for="native-input" class="d2l-input-label d2l-input-label-required">First Name</label>
<input id="native-input"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<input id="native-input"
<input
id="native-input"

render() {
return html`
<label for="nested-native-input">Name</label>
<input id="nested-native-input"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<input id="nested-native-input"
<input
id="nested-native-input"

Comment on lines +5 to +7
get isCustomFormElementContainer() {
return true;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort of blanking on how this might work. Is there a reason this can't just be a static property?

Copy link
Copy Markdown
Contributor

@bearfriend bearfriend May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static as in not a dynamic getter. Not static.


export const isCustomFormElement = (node) => isCustomElement(node) && !!node.formAssociated;

export const isCustomFormElementContainer = (node) => isCustomElement(node) && !!node.isCustomFormElementContainer;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export const isCustomFormElementContainer = (node) => isCustomElement(node) && !!node.isCustomFormElementContainer;
export const isCustomFormElementContainer = node => isCustomElement(node) && Boolean(node.isCustomFormElementContainer);

Comment on lines +30 to +36
const getElementChildren = (ele) => {
if (isCustomFormElementContainer(ele)) {
return ele.shadowRoot.children;
}

return ele.tagName === 'SLOT' && ['primary', 'secondary'].includes(ele.name) ? ele.assignedNodes() : ele.children;
};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typical convention is just el, or maybe elem for more public-facing API stuff I guess. Sort of a nit, but sort of not, for consistency.

Suggested change
const getElementChildren = (ele) => {
if (isCustomFormElementContainer(ele)) {
return ele.shadowRoot.children;
}
return ele.tagName === 'SLOT' && ['primary', 'secondary'].includes(ele.name) ? ele.assignedNodes() : ele.children;
};
const getElementChildren = el => {
if (isCustomFormElementContainer(el)) {
return el.shadowRoot.children;
}
return el.tagName === 'SLOT' && ['primary', 'secondary'].includes(el.name) ? el.assignedNodes() : el.children;
};

Comment on lines 38 to 45
const _findFormElementsHelper = (ele, eles, isFormElementPredicate, visitChildrenPredicate) => {
if (isNativeFormElement(ele) || isCustomFormElement(ele) || isFormElementPredicate(ele)) {
eles.push(ele);
}
if (visitChildrenPredicate(ele)) {
const children = ele.tagName === 'SLOT' && ['primary', 'secondary'].includes(ele.name) ? ele.assignedNodes() : ele.children;
const children = getElementChildren(ele);
for (const child of children) {
_findFormElementsHelper(child, eles, isFormElementPredicate, visitChildrenPredicate);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It won't let me make a suggestion here, but same thing with el. Maybe elems for the plural?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants