Follow-up from #1938910: Convert image theme tables to table #type.

CommentFileSizeAuthor
#18 edit_style_test_style.jpg41.67 KBmeramo
#15 move_add_new_effect_out-2116663-15.patch14.55 KBlammensj
#9 add_image_effect_button_vs_empty_text.png79.59 KBthijsvdanker
#7 move_add_new_effect_out-2116663-7.patch13.45 KBlammensj
#3 move_add_new_effect_out-2116663-3.patch9.17 KBJeroenT
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford commented
Issue summary: View changes
Status: Postponed » Active
JeroenT’s picture

JeroenT
Primary language Dutch
Location 🇧🇪
CreditAttribution: JeroenT commented
Assigned: Unassigned » JeroenT

I will work on this @ DrupalCon Amsterdam.

JeroenT’s picture

JeroenT
Primary language Dutch
Location 🇧🇪
CreditAttribution: JeroenT commented
Assigned: JeroenT » Unassigned
Status: Active » Needs review
FileSize
move_add_new_effect_out-2116663-3.patch9.17 KB

First try to move the new effect out of the effects table.

Patch attached.

Status: Needs review » Needs work

The last submitted patch, 3: move_add_new_effect_out-2116663-3.patch, failed testing.

lammensj’s picture

lammensj CreditAttribution: lammensj commented
Status: Needs work » Active

The patch works on the latest release but the test failed (obviously). I'm going to figure out why.

lammensj’s picture

lammensj CreditAttribution: lammensj commented
Assigned: Unassigned » lammensj
lammensj’s picture

lammensj CreditAttribution: lammensj commented
Assigned: lammensj » Unassigned
Status: Active » Needs review
FileSize
move_add_new_effect_out-2116663-7.patch13.45 KB

Fixed some minor bugs, according to the tests. Also updated the tests so they reflect the changes that JeroenT made.

thijsvdanker’s picture

thijsvdanker CreditAttribution: thijsvdanker commented
Assigned: Unassigned » thijsvdanker

I'm reviewing the patch.

thijsvdanker’s picture

thijsvdanker CreditAttribution: thijsvdanker commented
FileSize
add_image_effect_button_vs_empty_text.png79.59 KB

Before adding effects the table says "There are currently no effects in this style. Add one by selecting an option below."
The button is at the top of the screen, this is confusing. So I think we should either update the text or change the location of the button.

add style button

The test is also giving me an error

Fatal error: Call to a member function get() on a non-object in /var/www/drupal8/core/modules/image/src/Tests/ImageAdminStylesTest.php on line 302

I have to run now, so can't look into what's wrong there.

thijsvdanker’s picture

thijsvdanker CreditAttribution: thijsvdanker commented
Assigned: thijsvdanker » Unassigned
Status: Needs review » Needs work
slv_’s picture

slv_ CreditAttribution: slv_ commented

It seems to me this doesn't make sense anymore, since now this table has a dropdown and the 'add' button for the chosen effect in the last row, I guess as part of some issue to improve UX.

The last submitted patch, 7: move_add_new_effect_out-2116663-7.patch, failed testing.

lammensj’s picture

lammensj CreditAttribution: lammensj commented

I know how to fix this, just need to find some time. About the message, I suggest something like this: "There are currently no effects in this style". I think a reference to the button is not needed.

lammensj’s picture

lammensj CreditAttribution: lammensj commented
Assigned: Unassigned » lammensj

Fixing it now. Patch will soon be uploaded.

lammensj’s picture

lammensj CreditAttribution: lammensj commented
FileSize
move_add_new_effect_out-2116663-15.patch14.55 KB

Patch included fixes the tests and changes the placeholder to "There are currently no effects in this style.". Can someone confirm?

lammensj’s picture

lammensj CreditAttribution: lammensj commented
Assigned: lammensj » Unassigned
Status: Needs work » Needs review

Changing metadata of this issue.

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford commented
Issue tags: +Needs issue summary update
meramo’s picture

meramo CreditAttribution: meramo commented
Status: Needs review » Closed (won't fix)
Issue tags: +drupaldevdays
FileSize
edit_style_test_style.jpg41.67 KB

The interface has changed drastically since the last issue update (screenshot attached), and the patch is no longer applies for obvious reasons. Closing.