Problem/Motivation
The "classes" field on Styles entities uses a multivalued textfield with some ajax functionality
It's quite clumsy to remove classes, despite what the help text might make you think.
Also, the use of ajax makes functional testing more difficult. It also makes functional testing more important since it's custom ajax functionality rather than standard form API stuff.
Proposed resolution
It would make use and maintenance of this project easier if the ajax stuff were removed in favor of using a single form element to hold the multiple classes.
The single form element could be a textfield with space-delimited classes or a textarea with line-break delimited classes.
User interface changes
Simpler form
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#7 | interdiff-6-7.patch | 3.39 KB | danflanagan8 |
#7 | sylte_entity-form-3349078-7.patch | 7.32 KB | danflanagan8 |
#6 | interdiff_4-6.txt | 488 bytes | akshay_d |
#6 | refactor-classes-field-on-style-entity-3349078-6.patch | 3.94 KB | akshay_d |
Comments
Comment #2
akshay_dHi team,
I have updated the entity style classes field to have simpler form by updating the ajax enabled fieldset field to textfield.
Please review the PR
Thanks
Comment #3
danflanagan8Nice work, @akshay_d
This worked well in m y manual testing, but there are still a few changes I'd like to see.
First, I have a couple nitpicky things from the form build:
I think I'd rather see this:
$classes = $styles->get('classes') ?? [];
And then later we can implode without any additional logic:
That could be changed to:
'#default_value' => implode($classes)
And then I'd like to see the save method approached in a different way. We should override the parent's
copyFormValuesToEntity
method and deal with setting values in there. I think it would look like this:Then the save method would look like this:
Note we're returning the status at the end which is something we are forgetting to do at the moment.
This is a much better logical division of labor. Now the save method is only handling saving, messaging, and redirecting, which is what the docbloc says it should do.
And now the
copyFormValuesToEntity
will do what it says it does, and do it correctly the first time.Comment #4
akshay_dThanks @danflanagan8 for reviewing the patch.
Also, I made the requested change please re-review.
Comment #5
danflanagan8Looks good, @akshay_d. Thanks for the updates!
I just have one nit. It looks like the word "or" is missing from this sentence:
Can you change this to read, "Add one or more space-delimited classes."
Comment #6
akshay_d@danflanagan8 Thanks for the detailed review.
Also, Updated the patch please review.
Comment #7
danflanagan8Thanks, @akshay_d!
Here's an updated patch with tests. Ease of test coverage, after all, was part of the motivation here.
Comment #10
danflanagan8Ha! The interdiff mostly passed. :)