-
-
Notifications
You must be signed in to change notification settings - Fork 392
Add Collection component #398
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
base: 2.x
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| /.gitattributes export-ignore | ||
| /.gitignore export-ignore | ||
| /.symfony.bundle.yaml export-ignore | ||
| /phpunit.xml.dist export-ignore | ||
| /phpstan.neon.dist export-ignore | ||
| /Resources/assets/test export-ignore | ||
| /Resources/assets/jest.config.js export-ignore | ||
| /Tests export-ignore |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| /.php_cs.cache | ||
| /.php_cs | ||
| /.phpunit.result.cache | ||
| /composer.phar | ||
| /composer.lock | ||
| /phpunit.xml | ||
| /vendor/ | ||
| /Tests/app/var | ||
| /Tests/app/public/build/ | ||
| node_modules/ | ||
| package-lock.json | ||
| yarn.lock |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| branches: ["2.x"] | ||
| maintained_branches: ["2.x"] | ||
| doc_dir: "Resources/doc" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| # Contributing | ||
|
|
||
| Install the test app: | ||
|
|
||
| $ composer install | ||
| $ cd Tests/app | ||
| $ yarn install | ||
| $ yarn build | ||
|
|
||
| Start the test app: | ||
|
|
||
| $ symfony serve | ||
|
|
||
| ## Run tests | ||
|
|
||
| $ php vendor/bin/simple-phpunit |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| <?php | ||
|
|
||
| /* | ||
| * This file is part of the Symfony package. | ||
| * | ||
| * (c) Fabien Potencier <[email protected]> | ||
| * | ||
| * For the full copyright and license information, please view the LICENSE | ||
| * file that was distributed with this source code. | ||
| */ | ||
|
|
||
| namespace Symfony\UX\Collection; | ||
|
|
||
| use Symfony\Component\HttpKernel\Bundle\Bundle; | ||
|
|
||
| /** | ||
| * @author Kévin Dunglas <[email protected]> | ||
| */ | ||
| final class CollectionBundle extends Bundle | ||
| { | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| Copyright (c) 2021 Kévin Dunglas | ||
|
|
||
| Permission is hereby granted, free of charge, to any person obtaining a copy | ||
| of this software and associated documentation files (the "Software"), to deal | ||
| in the Software without restriction, including without limitation the rights | ||
| to use, copy, modify, merge, publish, distribute, sublicense, and/or sell | ||
| copies of the Software, and to permit persons to whom the Software is furnished | ||
| to do so, subject to the following conditions: | ||
|
|
||
| The above copyright notice and this permission notice shall be included in all | ||
| copies or substantial portions of the Software. | ||
|
|
||
| THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR | ||
| IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, | ||
| FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE | ||
| AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER | ||
| LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, | ||
| OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN | ||
| THE SOFTWARE. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| # Symfony UX Collection | ||
|
|
||
| **This repository is a READ-ONLY sub-tree split**. See | ||
| https://github.com/symfony/ux to create issues or submit pull requests. | ||
|
|
||
| ## Resources | ||
|
|
||
| - [Documentation](https://symfony.com/bundles/ux-collection/current/index.html) | ||
| - [Report issues](https://github.com/symfony/ux/issues) and | ||
| [send Pull Requests](https://github.com/symfony/ux/pulls) | ||
| in the [main Symfony UX repository](https://github.com/symfony/ux) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,87 @@ | ||
| import { Controller } from '@hotwired/stimulus'; | ||
|
|
||
| const DEFAULT_ITEMS_SELECTOR = ':scope > :is(div, fieldset)'; | ||
| var ButtonType; | ||
| (function (ButtonType) { | ||
| ButtonType[ButtonType["Add"] = 0] = "Add"; | ||
| ButtonType[ButtonType["Delete"] = 1] = "Delete"; | ||
| })(ButtonType || (ButtonType = {})); | ||
| class default_1 extends Controller { | ||
| connect() { | ||
| this.connectCollection(this.element); | ||
| } | ||
| connectCollection(parent) { | ||
| parent.querySelectorAll('[data-prototype]').forEach((el) => { | ||
| const collectionEl = el; | ||
| const items = this.getItems(collectionEl); | ||
| collectionEl.dataset.currentIndex = items.length.toString(); | ||
| this.addAddButton(collectionEl); | ||
| this.getItems(collectionEl).forEach((itemEl) => this.addDeleteButton(collectionEl, itemEl)); | ||
| }); | ||
| } | ||
| getItems(collectionElement) { | ||
| return collectionElement.querySelectorAll(collectionElement.dataset.itemsSelector || DEFAULT_ITEMS_SELECTOR); | ||
| } | ||
| createButton(collectionEl, buttonType) { | ||
| var _a; | ||
| const attributeName = `${ButtonType[buttonType].toLowerCase()}Button`; | ||
| const button = (_a = collectionEl.dataset[attributeName]) !== null && _a !== void 0 ? _a : this.element.dataset[attributeName]; | ||
| console.log(button); | ||
| if ('' === button) | ||
| return null; | ||
| if (undefined === button || !('content' in document.createElement('template'))) { | ||
| const button = document.createElement('button'); | ||
| button.type = 'button'; | ||
| button.textContent = ButtonType[buttonType]; | ||
| return button; | ||
| } | ||
| const buttonTemplate = document.getElementById(button); | ||
| if (!buttonTemplate) | ||
| throw new Error(`template with ID "${buttonTemplate}" not found`); | ||
| const fragment = buttonTemplate.content.cloneNode(true); | ||
| if (1 !== fragment.children.length) | ||
| throw new Error('template with ID "${buttonTemplateID}" must have exactly one child'); | ||
| return fragment.firstElementChild; | ||
| } | ||
| addItem(collectionEl) { | ||
| const currentIndex = collectionEl.dataset.currentIndex; | ||
| collectionEl.dataset.currentIndex++; | ||
| const collectionNamePattern = collectionEl.id.replace(/_/g, '(?:_|\\[|]\\[)'); | ||
| const prototype = collectionEl.dataset.prototype | ||
| .replace('__name__label__', currentIndex) | ||
| .replace(new RegExp(`(${collectionNamePattern}(?:_|]\\[))__name__`, 'g'), `$1${currentIndex}`); | ||
| const fakeEl = document.createElement('div'); | ||
| fakeEl.innerHTML = prototype; | ||
| const itemEl = fakeEl.firstElementChild; | ||
| this.connectCollection(itemEl); | ||
| this.addDeleteButton(collectionEl, itemEl); | ||
| const items = this.getItems(collectionEl); | ||
| items.length ? items[items.length - 1].insertAdjacentElement('afterend', itemEl) : collectionEl.prepend(itemEl); | ||
| } | ||
| addAddButton(collectionEl) { | ||
| const addButton = this.createButton(collectionEl, ButtonType.Add); | ||
| if (!addButton) | ||
| return; | ||
| addButton.onclick = (e) => { | ||
| e.preventDefault(); | ||
| this.addItem(collectionEl); | ||
| }; | ||
| collectionEl.appendChild(addButton); | ||
| } | ||
| addDeleteButton(collectionEl, itemEl) { | ||
| const deleteButton = this.createButton(collectionEl, ButtonType.Delete); | ||
| if (!deleteButton) | ||
| return; | ||
| deleteButton.onclick = (e) => { | ||
| e.preventDefault(); | ||
| itemEl.remove(); | ||
| }; | ||
| itemEl.appendChild(deleteButton); | ||
| } | ||
| } | ||
| default_1.values = { | ||
| addButton: '', | ||
| deleteButton: '', | ||
| }; | ||
|
|
||
| export { default_1 as default }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| module.exports = require('../../../../jest.config.js'); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| { | ||
| "name": "@symfony/ux-collection", | ||
| "description": "Support for collection embedding with Symfony Form", | ||
| "license": "MIT", | ||
| "main": "dist/controller.js", | ||
| "module": "dist/controller.js", | ||
| "version": "1.0.0", | ||
| "symfony": { | ||
| "controllers": { | ||
| "collection": { | ||
| "main": "dist/controller.js", | ||
| "webpackMode": "eager", | ||
| "fetch": "eager", | ||
| "enabled": true | ||
| } | ||
| } | ||
| }, | ||
| "peerDependencies": { | ||
| "@hotwired/stimulus": "^3.0.0" | ||
| }, | ||
| "devDependencies": { | ||
| "@hotwired/stimulus": "^3.0.0" | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,115 @@ | ||
| import { Controller } from '@hotwired/stimulus'; | ||
|
|
||
| const DEFAULT_ITEMS_SELECTOR = ':scope > :is(div, fieldset)'; | ||
|
|
||
| interface CollectionDataset extends DOMStringMap { | ||
| prototype: string; | ||
| currentIndex: string; | ||
| itemsSelector?: string; | ||
| addButton?: string; | ||
| deleteButton?: string; | ||
| } | ||
|
|
||
| enum ButtonType { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be a
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed the code and using
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another alternative is to use a tagged union |
||
| Add, | ||
| Delete, | ||
| } | ||
|
|
||
| export default class extends Controller { | ||
| static values = { | ||
| addButton: '', | ||
| deleteButton: '', | ||
| }; | ||
|
|
||
| connect() { | ||
| this.connectCollection(this.element as HTMLElement); | ||
| } | ||
|
|
||
| connectCollection(parent: HTMLElement) { | ||
| parent.querySelectorAll('[data-prototype]').forEach((el) => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still want to add here a comment that this the most questionable line in my point of view. In #400 I implemented the JS the way I think it should be implemented that every collection itself has its collection_controller and not that one controller controls all So we even don't need to wrap the whole form with a Why I would avoid the Via a form extension like in #400 we can add a better way by adding
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll make this selector configurable and split this controller in two (the main one in case you want all collections to be automatically handled, and a nested one). WDYT?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It still is hard to find here a correct selector as the main problems is that the component is registered on the As I did use component libraries over the last years which work very simular to What problem do you see that the |
||
| const collectionEl = el as HTMLElement; | ||
| const items = this.getItems(collectionEl); | ||
| collectionEl.dataset.currentIndex = items.length.toString(); | ||
|
|
||
| this.addAddButton(collectionEl); | ||
|
|
||
| this.getItems(collectionEl).forEach((itemEl) => this.addDeleteButton(collectionEl, itemEl as HTMLElement)); | ||
| }); | ||
| } | ||
|
|
||
| getItems(collectionElement: HTMLElement) { | ||
| return collectionElement.querySelectorAll(collectionElement.dataset.itemsSelector || DEFAULT_ITEMS_SELECTOR); | ||
| } | ||
|
|
||
| createButton(collectionEl: HTMLElement, buttonType: ButtonType): HTMLElement | null { | ||
| const attributeName = `${ButtonType[buttonType].toLowerCase()}Button`; | ||
| const button = collectionEl.dataset[attributeName] ?? (this.element as HTMLElement).dataset[attributeName]; | ||
| console.log(button); | ||
|
|
||
| // Button explicitly disabled through data attribute | ||
| if ('' === button) return null; | ||
|
|
||
| // No data attribute provided or <template> not supported: create raw HTML button | ||
| if (undefined === button || !('content' in document.createElement('template'))) { | ||
| const button = document.createElement('button') as HTMLButtonElement; | ||
| button.type = 'button'; | ||
| button.textContent = ButtonType[buttonType]; | ||
|
|
||
| return button; | ||
| } | ||
|
|
||
| // Use the template referenced by the data attribute | ||
| const buttonTemplate = document.getElementById(button) as HTMLTemplateElement | null; | ||
| if (!buttonTemplate) throw new Error(`template with ID "${buttonTemplate}" not found`); | ||
|
|
||
| const fragment = buttonTemplate.content.cloneNode(true) as DocumentFragment; | ||
| if (1 !== fragment.children.length) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this could use |
||
| throw new Error('template with ID "${buttonTemplateID}" must have exactly one child'); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this needs to use backticks, not single quotes. Otherwise the interpolation won't work. |
||
|
|
||
| return fragment.firstElementChild as HTMLElement; | ||
| } | ||
|
|
||
| addItem(collectionEl: HTMLElement) { | ||
| const currentIndex = (collectionEl.dataset as CollectionDataset).currentIndex; | ||
| (collectionEl.dataset.currentIndex as unknown as number)++; | ||
|
|
||
| const collectionNamePattern = collectionEl.id.replace(/_/g, '(?:_|\\[|]\\[)'); | ||
|
|
||
| const prototype = (collectionEl.dataset.prototype as string) // We're sure that dataset.prototype exists, because of the CSS selector used in connect() | ||
| .replace('__name__label__', currentIndex) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually my code supports nested collections even when not using another placeholder, but I'll add support for this option!
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No it does not. The prototype of the outer collection actually contains the attribute storing the prototype of the inner collection (as it contains the whole inner collection). If you use the same placeholder for both, the replacement done here will replace both placeholders inside the inner prototype, breaking the inner collection.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Try doing a nested collection where you add multiple items in the inner collection of an item added in the outer collection, and then look at the submitted data in the Request (you need to inspect the request itself, not the data decoded in
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I didn't handle the label yet, but for
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. replacing only the first occurrence is also broken.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The first occurence in every string: https://github.com/symfony/ux/pull/398/files#diff-0c84303846c77e8d3377b23960aed4a1dfc3db0bb6206c7435ef6fe890a7fa1bR49 Unless I'm missing something, this works with the current example I provided for test purpose.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dunglas AFAICT, this would replace both placecholders in Note that you need to add multiple items in each collection to notice the issue (if you only add one outer item with a single inner item, you won't notice whether the |
||
| .replace(new RegExp(`(${collectionNamePattern}(?:_|]\\[))__name__`, 'g'), `$1${currentIndex}`); | ||
|
|
||
| const fakeEl = document.createElement('div'); | ||
| fakeEl.innerHTML = prototype; | ||
| const itemEl = fakeEl.firstElementChild as HTMLElement; | ||
|
|
||
| this.connectCollection(itemEl); | ||
|
|
||
| this.addDeleteButton(collectionEl, itemEl); | ||
|
|
||
| const items = this.getItems(collectionEl); | ||
| items.length ? items[items.length - 1].insertAdjacentElement('afterend', itemEl) : collectionEl.prepend(itemEl); | ||
| } | ||
|
|
||
| addAddButton(collectionEl: HTMLElement) { | ||
| const addButton = this.createButton(collectionEl, ButtonType.Add); | ||
| if (!addButton) return; | ||
|
|
||
| addButton.onclick = (e) => { | ||
| e.preventDefault(); | ||
| this.addItem(collectionEl); | ||
| }; | ||
| collectionEl.appendChild(addButton); | ||
| } | ||
|
|
||
| addDeleteButton(collectionEl: HTMLElement, itemEl: HTMLElement) { | ||
| const deleteButton = this.createButton(collectionEl, ButtonType.Delete); | ||
| if (!deleteButton) return; | ||
|
|
||
| deleteButton.onclick = (e) => { | ||
| e.preventDefault(); | ||
| itemEl.remove(); | ||
| }; | ||
| itemEl.appendChild(deleteButton); | ||
| } | ||
| } | ||
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.
2022? 😌