Skip to content

feat: control container option #428

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.

Sign up for GitHub

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

Open
kevinchappell wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: control container option #428

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3499cb8
feat: use controls container option
kevinchappell Jan 30, 2026
114b8ca
chore: add test for resolveContainer
kevinchappell Jan 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/lib/js/common/dom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -944,6 +944,10 @@ class DOM {
!!(variable && typeof variable === 'object' && variable.nodeType === 1 && typeof variable.nodeName === 'string')
)
}

resolveContainer(container) {
return typeof container === 'string' ? document.querySelector(container) : container
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

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

resolveContainer currently returns any non-string value as-is and doesn’t guard document.querySelector against invalid selector strings (which throw). This can lead to hard-to-debug runtime errors later (e.g., calling DOM APIs on a non-element) or immediate initialization failures for malformed selectors. Consider validating with isDOMElement and returning null (or throwing a clear error) for unsupported types, and wrapping querySelector in a try/catch to return null for invalid selectors.

Suggested change
return typeof container === 'string' ? document.querySelector(container) : container
if (typeof container === 'string') {
try {
return document.querySelector(container)
} catch (e) {
// Invalid selector string
return null
}
}
if (this.isDOMElement(container)) {
return container
}
return null

Copilot uses AI. Check for mistakes.
}
}

export const dom = new DOM()
Expand Down
30 changes: 30 additions & 0 deletions src/lib/js/common/dom.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,36 @@ describe('DOM Class', async _t => {
assert.equal(dom.isDOMElement(null), false)
})

await test('resolveContainer', () => {
// Create a test element
const testDiv = document.createElement('div')
testDiv.id = 'test-resolve-container'
document.body.appendChild(testDiv)

// Test with string selector
const resolvedBySelector = dom.resolveContainer('#test-resolve-container')
assert.equal(resolvedBySelector, testDiv)

// Test with DOM element
const resolvedByElement = dom.resolveContainer(testDiv)
assert.equal(resolvedByElement, testDiv)

// Test with null
const resolvedNull = dom.resolveContainer(null)
assert.equal(resolvedNull, null)

// Test with undefined
const resolvedUndefined = dom.resolveContainer(undefined)
assert.equal(resolvedUndefined, undefined)

// Test with non-existent selector
const resolvedNonExistent = dom.resolveContainer('#non-existent-element')
assert.equal(resolvedNonExistent, null)

// Cleanup
document.body.removeChild(testDiv)
})

await test('create', async _t => {
await test('should return undefined for falsy input', () => {
assert.equal(dom.create(), undefined)
Expand Down
2 changes: 1 addition & 1 deletion src/lib/js/components/controls/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ export class Controls {

applyOptions = async (controlOptions = {}) => {
const { container, elements, groupOrder, ...options } = merge(defaultOptions, controlOptions)
this.container = container
this.container = dom.resolveContainer(container)
this.groupOrder = unique(groupOrder.concat(['common', 'html', 'layout']))
this.options = options

Expand Down
4 changes: 3 additions & 1 deletion src/lib/js/config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ export const defaults = {
config: {}, // stages, rows, columns, fields
events: {},
actions: {},
controls: {},
controls: {
container: null, // element or selector to attach controls to
},
i18n: {
location: 'https://draggable.github.io/formeo/assets/lang/',
},
Expand Down
8 changes: 5 additions & 3 deletions src/lib/js/editor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,7 @@ export class FormeoEditor {

const { actions, events, debug, config, editorContainer, ...opts } = mergedOptions
if (editorContainer) {
this.editorContainer =
typeof editorContainer === 'string' ? document.querySelector(editorContainer) : editorContainer
this.editorContainer = dom.resolveContainer(editorContainer) || null
}
this.opts = opts
dom.setOptions = opts
Expand Down Expand Up @@ -317,7 +316,10 @@ export class FormeoEditor {
this.editor = dom.create(elemConfig)

const controlsContainer = this.controls.container || this.editor
controlsContainer.appendChild(this.controls.dom)
if (controlsContainer) {
dom.empty(controlsContainer)
controlsContainer.appendChild(this.controls.dom)
}
Comment on lines 318 to +322
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

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

dom.empty(controlsContainer) will clear the editor root when controls.container is not set (default null), because controlsContainer falls back to this.editor. That removes the stage DOM nodes you just created in elemConfig.content, so the rendered editor will only contain the controls. Instead of emptying unconditionally, only clear/replace when an external controls.container is configured (or remove/replace an existing .formeo-controls node) and leave this.editor content intact.

Copilot uses AI. Check for mistakes.
Comment on lines 318 to +322
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

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

This change introduces/activates controls.container behavior, but there doesn’t appear to be any Playwright coverage asserting that controls render into a custom container (and continue to render correctly on re-render). Adding an e2e test that configures controls.container (and optionally the interaction with editorContainer) would help prevent regressions here.

Copilot uses AI. Check for mistakes.

if (this.editorContainer) {
dom.empty(this.editorContainer)
Comment on lines +321 to 325
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

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

If controls.container resolves to the same element as editorContainer, controls are appended and then immediately removed when dom.empty(this.editorContainer) runs (and similarly if the controls container is inside editorContainer, it gets removed by the empty call). Consider handling the “shared container” case by clearing once and appending both controls + editor in the intended order, or documenting/enforcing that these containers must be distinct/dedicated.

Copilot uses AI. Check for mistakes.
Expand Down
Loading