-
-
Notifications
You must be signed in to change notification settings - Fork 398
[LiveComponent] Fix expanded choices and checkboxes in forms #221
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
Changes from 32 commits
e76bdb9
76de2a2
4a37d10
fc953dd
841e07f
cab4c4a
f5e34fa
cfbbf9d
255d9f9
2f1d0ee
a17870f
0f4e70e
71f49ae
546c5a7
66f85e8
d183645
1e31c30
6d41d90
296f9a4
ffd6b72
a5f56f2
18741bd
a194f0c
7580a73
f342a7a
e51f202
56caf14
f87796d
3204490
d00cb41
4c8336b
a636a71
f13a498
914bcd5
d195ee9
f7dbdc1
bd05418
857ff1f
33dabc0
eb92ebd
2f735a2
d96d82f
2de0cbf
7605821
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,39 @@ | ||
| /** | ||
| * Resolve multiple value data from changed HTML element. | ||
| * | ||
| * @param element Current HTML element | ||
| * @param value Resolved value of a single HTML element (.value or [data-value]) | ||
Lustmored marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| * @param currentValue Current data value | ||
| */ | ||
| export function getArrayValue( | ||
Lustmored marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| element: HTMLElement, | ||
| value: string|null, | ||
| currentValue: any | ||
Lustmored marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| ): Array<string> { | ||
| // Enforce returned value is an array | ||
| if (!(currentValue instanceof Array)) { | ||
| currentValue = []; | ||
| } | ||
|
|
||
| if (element instanceof HTMLInputElement && element.type === 'checkbox') { | ||
weaverryan marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| const index = currentValue.indexOf(value); | ||
|
|
||
| if (element.checked) { | ||
| // Add value to an array if it's not in it already | ||
| if (index === -1) { | ||
| currentValue.push(value); | ||
| } | ||
| } else { | ||
| // Remove value from an array | ||
| if (index > -1) { | ||
| currentValue.splice(index, 1); | ||
| } | ||
| } | ||
Lustmored marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } else if (element instanceof HTMLSelectElement) { | ||
weaverryan marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| // Select elements with `multiple` option require mapping chosen options to their values | ||
| currentValue = Array.from(element.selectedOptions).map(el => el.value); | ||
| } | ||
weaverryan marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| // When no values are selected for collection no data should be sent over the wire | ||
| return currentValue; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,10 +2,11 @@ import { Controller } from '@hotwired/stimulus'; | |
| import morphdom from 'morphdom'; | ||
| import { parseDirectives, Directive } from './directives_parser'; | ||
| import { combineSpacedArray } from './string_utils'; | ||
| import { setDeepData, doesDeepPropertyExist, normalizeModelName } from './set_deep_data'; | ||
| import {setDeepData, doesDeepPropertyExist, normalizeModelName, parseDeepData} from './set_deep_data'; | ||
Lustmored marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| import { haveRenderedValuesChanged } from './have_rendered_values_changed'; | ||
| import { normalizeAttributesForComparison } from './normalize_attributes_for_comparison'; | ||
| import { cloneHTMLElement } from './clone_html_element'; | ||
| import {getArrayValue} from "./get_array_value"; | ||
Lustmored marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| interface ElementLoadingDirectives { | ||
| element: HTMLElement, | ||
|
|
@@ -105,15 +106,11 @@ export default class extends Controller { | |
| * Called to update one piece of the model | ||
| */ | ||
| update(event: any) { | ||
| const value = this._getValueFromElement(event.target); | ||
|
|
||
| this._updateModelFromElement(event.target, value, true); | ||
| this._updateModelFromElement(event.target, this._getValueFromElement(event.target), true); | ||
| } | ||
|
|
||
| updateDefer(event: any) { | ||
| const value = this._getValueFromElement(event.target); | ||
|
|
||
| this._updateModelFromElement(event.target, value, false); | ||
| this._updateModelFromElement(event.target, this._getValueFromElement(event.target), false); | ||
| } | ||
|
|
||
| action(event: any) { | ||
|
|
@@ -194,7 +191,7 @@ export default class extends Controller { | |
| return element.dataset.value || (element as any).value; | ||
| } | ||
|
|
||
| _updateModelFromElement(element: HTMLElement, value: string, shouldRender: boolean) { | ||
| _updateModelFromElement(element: HTMLElement, value: string|null, shouldRender: boolean) { | ||
| const model = element.dataset.model || element.getAttribute('name'); | ||
|
|
||
| if (!model) { | ||
|
|
@@ -203,7 +200,25 @@ export default class extends Controller { | |
| throw new Error(`The update() method could not be called for "${clonedElement.outerHTML}": the element must either have a "data-model" or "name" attribute set to the model name.`); | ||
| } | ||
|
|
||
| this.$updateModel(model, value, shouldRender, element.hasAttribute('name') ? element.getAttribute('name') : null); | ||
| // HTML form elements with name ending with [] require array as data | ||
| // we need to handle addition and removal of values from it to send | ||
| // back only required data | ||
| if (/\[]$/.test(model)) { | ||
| // Get current value from data | ||
| const {currentLevelData, finalKey} = parseDeepData(this.dataValue, normalizeModelName(model)) | ||
| const currentValue = currentLevelData[finalKey]; | ||
|
|
||
| value = getArrayValue(element, value, currentValue); | ||
| } else if ( | ||
| element instanceof HTMLInputElement | ||
weaverryan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| && element.type === 'checkbox' | ||
| && !element.checked | ||
| ) { | ||
| // Unchecked checkboxes in a single value scenarios should map to `null` | ||
| value = null; | ||
| } | ||
|
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. If I understand things correctly, this handles a case where, for example, we have an If so, I'd love to isolate this logic to an external module/file - like in
Contributor
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 just did as you asked - great call! Resulting function is much simpler and cleaner in my opinion and it was extremely easy to unit test it. I have already incorporate "select multiple" case within it, so it should start working soon. Still some test to write and I'll manually check how it behaves too, but for the first time I can see the finish line 😄
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 is all reading VERY nicely now. It's a complex situation, but I can understand it - nice work! |
||
|
|
||
| this.$updateModel(model, value, shouldRender, element.hasAttribute('name') ? element.getAttribute('name') : null, {}); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -253,7 +268,7 @@ export default class extends Controller { | |
| this._dispatchEvent('live:update-model', { | ||
| modelName, | ||
| extraModelName: normalizedExtraModelName, | ||
| value, | ||
| value | ||
| }); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,5 @@ | ||
| // post.user.username | ||
| export function setDeepData(data, propertyPath, value) { | ||
| // cheap way to deep clone simple data | ||
| export function parseDeepData(data, propertyPath) { | ||
| const finalData = JSON.parse(JSON.stringify(data)); | ||
|
|
||
| let currentLevelData = finalData; | ||
|
|
@@ -14,6 +13,18 @@ export function setDeepData(data, propertyPath, value) { | |
| // now finally change the key on that deeper object | ||
| const finalKey = parts[parts.length - 1]; | ||
|
|
||
| return { | ||
| currentLevelData, | ||
| finalData, | ||
| finalKey, | ||
| parts | ||
| } | ||
| } | ||
|
|
||
| // post.user.username | ||
| export function setDeepData(data, propertyPath, value) { | ||
| const {currentLevelData, finalData, finalKey, parts} = parseDeepData(data, propertyPath) | ||
Lustmored marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| // make sure the currentLevelData is an object, not a scalar | ||
| // if it is, it means the initial data didn't know that sub-properties | ||
| // could be exposed. Or, you're just trying to set some deep | ||
|
|
@@ -64,9 +75,14 @@ export function doesDeepPropertyExist(data, propertyPath) { | |
| */ | ||
| export function normalizeModelName(model) { | ||
| return model | ||
| // Names ending in "[]" represent arrays in HTML. | ||
| // To get normalized name we need to ignore this part. | ||
| // For example: "user[mailing][]" becomes "user.mailing" (and has array typed value) | ||
| .replace(/\[]$/, '') | ||
|
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. Can you add a test to the bottom of
Contributor
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 will paste here comment as I'm adding it:
I have also added simple test case |
||
| .split('[') | ||
| // ['object', 'foo', 'bar', 'ya'] | ||
| .map(function (s) { | ||
| return s.replace(']', '') | ||
| }).join('.') | ||
| }) | ||
| .join('.') | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.