The "Remove one" button makes it difficult to manage denominations in the form. Instead, there should be a remove button for each denomination, that way if you need to remove one that's higher up in the list you don't need to remove and re-add all the others that come after it.
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | interdiff-2906918-12-14.txt | 674 bytes | rakesh.gectcr |
| #15 | 2906918-14.patch | 5.54 KB | rakesh.gectcr |
| #12 | the_remove_one_button-2906918-12.patch | 5.52 KB | shabana.navas |
| #10 | the_remove_one_button-2906918-10.patch | 4.83 KB | shabana.navas |
| #8 | Screen Shot 2017-09-21 at 4.35.47 PM.png | 134.99 KB | shabana.navas |
Comments
Comment #2
TylerMarshall commentedAlso issue related, would probably get solved in this, but if you Add a denomination, but want to remove it, you can't because it doesn't exist.
Comment #3
TylerMarshall commentedComment #4
pbrown063 commentedWhere I am at sofar with this patch:
- Moved the 'Remove' button into the array
- Fiddled with the 'removeCallback' (with little success)
What needs to be done:
- Determine how to extract the index number from $form['denominations'] on button click
- Use that number to delete the entry in the CurrencyDenominationsList
Comment #5
TylerMarshall commentedI think we will need to rebuild the way the denominations Add works, to also use a more OOO approach. This would let use use proper ajax commands. Have a WIP
Comment #6
rakesh.gectcrIMHO, There is not much Difference about the Form APIs works in Drupal 7 and Drupal 8
Comment #7
pbrown063 commentedUpdated patch with diff files.
Where I have it in this state:
- Indexed each 'remove' button and gave it a unique value
- removeCallback now modifies the $form directly
What needs to be done:
- The changes that I am making to the form directly are being overwritten on a rebuild
- And that override needs to be prevented or utilised with these changes.
Comment #8
shabana.navas commentedJust tested this and you're right, the values are getting overwritten/lost. There's also an issue where when we click on 'Add one more' the remove button's label title changes to 'Remove Array'. The first thing is
should be
I checked it further and $denom is coming up empty after that ajax call so trying to retrieve this:
comes up empty.
Comment #9
shabana.navas commentedComment #10
shabana.navas commentedA couple of changes to the earlier patch which fixes the issue with the Remove button coming up with empty denominations whenever the 'Add one more' button was clicked.
Comment #11
shabana.navas commentedA fully working and more refined way to add/remove denominations.
Comment #12
shabana.navas commentedOops posted wrong patch earlier.
Comment #13
shabana.navas commentedComment #14
rakesh.gectcrWell the patch perfectly fine, Need a small change on
'#value' => t('Remove') . " " . $default_label,We should not concatenate strings to translatable strings
Its hould be in following way
'#value' => t('Remove @default_label', ['@default_label' => $default_label]),Comment #15
rakesh.gectcrThanks to all for the above patches. I am adding a quick patch for above one.
So I am moving into RTBC.
Comment #16
rakesh.gectcrI will move to RTBC, ones the test get pass.
Comment #18
TylerMarshall commentedLooks good to me!
Comment #19
TylerMarshall commentedEr testing updated for me to not passed. Will check that first.
Comment #20
smccabe commented@Tyler_Marshall, the test failure was just on the interdiff triggering testing, the actual patch passed correctly.
Comment #22
smccabe commented