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

Fix/multi select field grouped #1830

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

SJost
Copy link

@SJost SJost commented Dec 11, 2023

fix multiSelectField being partial

Constructor OptionListGrouped could not be processed by function multiSelectField. This patch fixes this, using tags within the generated Html.


Before submitting your PR, check that you've:

  • Bumped the version number

After submitting your PR:

  • Update the Changelog.md file with a link to your PR
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

Constructor OptionListGrouped could not be processed by function multiSelectField. This patch fixes this, using <optgroup> tags within the generated Html.
@schoettl
Copy link
Contributor

Oh, thanks for fixing this! I missed to handle that case when I introduced OptionListGrouped in 2021.

@SJost
Copy link
Author

SJost commented Dec 12, 2023

@schoettl Happy to help. Though I am not sure what to do with the remaining open instructions to update the Changelog.md? How do I know which version the PR is merged into, if at all?

@SJost
Copy link
Author

SJost commented Dec 12, 2023

Furthermore, in the version that I am actually using, I have an additional Maybe (SomeMessage site) argument that is used as the None Option, if isReq is False, since my users had problems with de-selecting.

I could add versions selectField' and multiSelectField' with this option, if desired, but I thought it might only dilute this PR.

@schoettl
Copy link
Contributor

Furthermore, in the version that I am actually using, I have an additional Maybe (SomeMessage site) argument that is used as the None Option, if isReq is False, since my users had problems with de-selecting.

You mean your users have problems to clear the selection in the multi select field, which is usually done in desktop browsers by Ctrl + click? TBH, I would leave this to the developer and not provide an extra implementation for that.

Regarding the open checkboxes, I think you can just add a note in the changelog for the new version you set.

And it looks to me as if CI would be started at approval of a maintainer. I think it has timed-out too often.

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

Successfully merging this pull request may close these issues.

2 participants