Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Remove JS from checkbox #3043

Closed
wants to merge 2 commits into from
Closed

Remove JS from checkbox #3043

wants to merge 2 commits into from

Conversation

martijnb92
Copy link
Contributor

Remove unnecessary javascript from the checkbox field.
Same can be accomplished without it.

@pxpm
Copy link
Contributor

pxpm commented Jul 17, 2020

@martijnb92 I am not 100% sure we can use id's in fields because of repeatable. I think using ID breaks this field in repeatable.

@martijnb92
Copy link
Contributor Author

@martijnb92 I am not 100% sure we can use id's in fields because of repeatable. I think using ID breaks this field in repeatable.

Good one! Removed the ID and wrapped the label around the checkbox.

@scrutinizer-notifier
Copy link

The inspection completed: No new issues

@tabacitu
Copy link
Member

Yup, what @pxpm said 😄 I'm pretty sure without the JS this won't work well inside repeatable @martijnb92 , sorry. For example, if we do this, the checkbox inside repeatable is no longer checked when it should be:
2020-07-20 12 09 30

To rephrase - I agree, this all can be done without JS. And it's the way it's been in Backpack 1.0.0-4.0.x. But in Backpack 4.1 it's been rewritten to use Javascript specifically so it works inside the repeatable field type. Happy to polish the code - I too believe less is more, but in this case we need the JS so I'm going to close this @martijnb92 , sorry, because it breaks functionality.

Let me know if you find a way to have it working inside repeatable without JS and we'll reopen - personally I didn't.

Thanks for the PR nonetheless - really appreciate the time you put in. Cheers!

@tabacitu tabacitu closed this Jul 20, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants