-
Notifications
You must be signed in to change notification settings - Fork 4.5k
🧹 Transform Template value on Transformation Details #29676
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
Conversation
|
CI Results: failed ❌ |
|
Build Results: |
| }) | ||
| masking_character; | ||
|
|
||
| // TODO: add presence required validation for template |
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.
This is the only real change on the model, everything else is due to glimmerizing
| // For model attributes using editType: 'searchSelect' where the API returns a string, | ||
| // we need to wrap the value in an array to match the format expected by the dropdownOptions. | ||
| // Example: model/transform.js -> template | ||
| inputValues = Array.isArray(inputValues) ? inputValues : [inputValues]; |
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.
Can we add a test for this behavior? Search select is a pretty bloated component as is, so I'm nervous to add even seemingly small logic to it.
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.
Test coverage is incoming. It's a mess in there, so I stayed with selector patterns that existed and added coverage for search select but also acceptance test coverage for the create/edit workflow.
hellobontempo
left a comment
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.
A few thoughts here, if you want to go this route. I definitely think we need a test.
Alternatively, it seems like the edit type in the model was "array" may have been to leverage built-in ember data transform magic ✨ . Or there's also a templates attribute returned by the API (docs), maybe the model was supposed to reference that parameter and should be plural
Also, I'm not seeing an issue locally - what's the original bug that's being fixed here? The transform template seems to edit just fine 🤔
| assert.dom('.list-item-text').exists('selected option field has width set'); | ||
| assert.dom('.text-overflow-ellipsis').exists('selected option text has overflow class'); | ||
| }); | ||
|
|
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.
inputValue is the only argument passed into the formatInputAndUpdateDropdown method that was modified in this PR. In this test, we're testing when inputValue is a string. InputValue as an array is tested in the test above this here
|
Not a priority, more of a rabbit hole, type situation will be resolved in later ember data work. |
Description
What was originally intended as a simple CSS cleanup revealed an issue with how we were using model attributes
editType: searchSelectwithtype: string. These two attributes were incompatible during an edit scenario. There is one other case where these two attributes are used together, but in that case, theeditDisabled: trueflag prevented any issues from surfacing during editing.In the
model/transform.jsthetemplateattribute was incorrectly labeled astype="array". According to the API here,templateshould be type "string". Updating the model type fixed the styling issue, but it broke the edit functionality.To resolve the edit issue, I modified the method responsible for reading the attribute’s value in the
searchSelectcomponent. If the value wasn't already an array, I converted it into an array so the function could process it correctly. This approach seemed to be the least disruptive solution.Before:

After:
