Skip to content
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

EZP-30164: After removing option from ezselection it it not possible to add more options #894

Merged
merged 3 commits into from
Mar 13, 2019

Conversation

ViniTou
Copy link
Contributor

@ViniTou ViniTou commented Mar 12, 2019

Question Answer
Tickets https://jira.ez.no/browse/EZP-30164
Bug fix? yes
New feature? no
BC breaks? no
Tests pass? yes
Doc needed? no
License GPL-2.0

Instead of counting options, which creates problem with overlapping ids when prior to add option is removed, we use id extracted from name from last input.

Checklist:

  • Coding standards ($ composer fix-cs)
  • Ready for Code Review

@ViniTou ViniTou changed the title EZP-30164: After removing option from ezselection it it not possible … EZP-30164: After removing option from ezselection it it not possible to add more options Mar 12, 2019
return 0;
}
const lastInput = container.querySelector(SELECTOR_OPTIONS_LIST).lastElementChild.querySelector('input[type="text"]');
const lastId = parseInt(lastInput.name.match(/.+\[(\d)]$/)[1]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be stored in data-attribute. The same like @Nattfarinn did in ezmatrix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dew326
fixd

@ViniTou ViniTou force-pushed the EZP-30164-ezselection-remove-options branch from 0b7ac0e to f51c826 Compare March 12, 2019 14:40
@barbaragr barbaragr self-assigned this Mar 12, 2019
{{ form_widget(option, { 'attr': { 'class': 'form-control ml-1 mb-0'}}) }}
{{ form_widget(option, { 'attr': {
'class': 'form-control ml-1 mb-0',
'data-ezselection-option-id': option.vars.name }})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant more like adding data-attribute only to the parent ezselection-settings like data-last-id={{ max(form.options|keys) }}.

if (countExistingOptions() === 0) {
return 0;
}
const lastInput = container.querySelector(SELECTOR_OPTIONS_LIST).lastElementChild.querySelector('input[type="text"]');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This query will be much simpler if you use what is proposed in previous comment

const getNextId = () => {
if (countExistingOptions() === 0) {
return 0;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after if there is always empty line

@@ -15,12 +15,21 @@

container.querySelector(SELECTOR_BTN_REMOVE)[methodName]('disabled', disabledState);
};
const getNextId = () => {
if (countExistingOptions() === 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can use just !countExistingOptions()

const lastInput = container.querySelector(SELECTOR_OPTIONS_LIST).lastElementChild.querySelector('input[type="text"]');
const lastId = lastInput.dataset.ezselectionOptionId;

return lastId + 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the ezselectionOptionId is '3' this will return '31'. I believe it's not what you want ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don;t know how I missed parseInt, it was here before :D

@ViniTou ViniTou requested a review from dew326 March 13, 2019 09:34
@lserwatka lserwatka merged commit 2e4261c into ezsystems:1.4 Mar 13, 2019
@lserwatka
Copy link
Member

Could you merge it up?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants