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

M2 4786 #1085

Closed
wants to merge 7 commits into from
Closed

M2 4786 #1085

wants to merge 7 commits into from

Conversation

childmindhirsch
Copy link

Changes

This PR introduces the "None of the above" option for multi-select fields, which allows users to deselect other options when this is selected.

Implementation

  • Added a new checkbox option with a special is_none_above property.
  • Updated the session storage logic to retain the state of this option

@Damirkhon
Copy link
Contributor

Damirkhon commented Feb 12, 2024

Hi @childmindhirsch, great to see you in the project.
I reviewed your changes, and have comments on the implementation:

  1. You need to change the response_values models for MultiSelect, rather than config. Here you can see
    that _SingleSelectionValue is used for both singleselect and multiselect. But now, because we have difference between them we need to separate the models. Something like this:
class _MultiSelectionValue(_SingleSelectionValue, PublicModel):
    is_none_above: bool = Field(default=False)

And use it inside MultiSelectionValues class, probably like this:

class MultiSelectionValues(PublicModel):
    palette_name: str | None
    options: list[_MultiSelectionValue]
    
    @validator("options")
    def validate_options(cls, value):
        return validate_options_value(value)
  1. Don't forget to implement another validation for it, that options inside MultiSelectionValues should not contain more than 1 option with is_none_above as true
  2. Please, change current or add new unit and integration tests as well.

If you have any questions, feel free to contact via slack

@@ -71,12 +71,8 @@ class _SelectionConfig(_ScreenConfig, PublicModel):
additional_response_option: AdditionalResponseOption


class SingleSelectionConfig(_SelectionConfig, PublicModel):
Copy link
Contributor

Choose a reason for hiding this comment

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

don't delete this config

@Damirkhon
Copy link
Contributor

Re-implemented by #1089

@Damirkhon Damirkhon closed this Feb 14, 2024
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