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: fixed handling of how and when to set ALL option #7244

Merged
merged 3 commits into from
Mar 10, 2025
Merged

Conversation

SagarRajput-7
Copy link
Contributor

@SagarRajput-7 SagarRajput-7 commented Mar 8, 2025

Summary

Changes

To incorporate the advantages of restricting values while avoiding the issue from the previous implementation, I have made the following changes regarding selection and retention:

ALL value selected will remain as ALL until an unless the selectedValue array is equal to the options coming from BE, as when the timeframe changes, there might be cases when the options will decrease or increase, hence,

  • When all the options coming from BE are present in selectedValue, then it will show as all
  • but in case we have a newer addition that is not present in the previously selected value, then we can't show it as selected, as the user might not want to select that new option
  • Specific selections, let's say 3/10 variables selected, will also retain
  • empty variable selection will also be retained

The main goal is to provide a natural, intuitive user experience and avoid unexpected modifications from our side.

Related Issues / PR's

Screenshots

Current issue with ALL label selection:

Screen.Recording.2025-03-08.at.2.00.26.PM.mov

Fix:

Screen.Recording.2025-03-08.at.2.01.59.PM.mov

Affected Areas and Manually Tested Areas

Tested -

Tests -

  • ALL value selected remain ALL until the every value in option from BE is present in selectedValue array
  • Specific selections remain
  • empty selection is retained

Important

Improved handling of 'ALL' option in multi-select scenarios in VariableItem.tsx by updating logic to set allSelected and handling empty input values.

  • Behavior:
    • In VariableItem.tsx, added logic to set allSelected when all options are selected in multi-select mode.
    • Updated handleChange to handle empty input values for multi-select by setting value to an empty array.
  • Functions:
    • Modified getOptions to determine allSelected by comparing lengths and contents of selectedValue and newOptionsData.
    • Updated handleChange to handle multiSelect scenarios with empty input values.

This description was created by Ellipsis for ad06b83. It will automatically update as commits are pushed.

@SagarRajput-7 SagarRajput-7 requested a review from YounixM as a code owner March 8, 2025 08:04
@github-actions github-actions bot added the bug Something isn't working label Mar 8, 2025
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to ad06b83 in 1 minute and 33 seconds

More details
  • Looked at 30 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 6 drafted comments based on config settings.
1. frontend/src/container/NewDashboard/DashboardVariablesSelection/VariableItem.tsx:161
  • Draft comment:
    In the new multiSelect branch, the logic that checks if all options are selected is valid but could be more robust. Consider edge cases where newOptionsData order or duplicates might affect the check.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
2. frontend/src/container/NewDashboard/DashboardVariablesSelection/VariableItem.tsx:246
  • Draft comment:
    Renaming the parameter to inputValue improves clarity. Ensure that for multiSelect, an empty input correctly defaults to an empty array as intended.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is asking the author to ensure a specific behavior, which violates the rule against asking the author to ensure intended behavior. It doesn't provide a specific suggestion or ask for a test to be written, so it should be removed.
3. frontend/src/container/NewDashboard/DashboardVariablesSelection/VariableItem.tsx:161
  • Draft comment:
    Consider extracting the 'allSelected' computation into a helper function. This would improve readability and reduce cognitive complexity, especially since similar logic might be needed elsewhere.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
4. frontend/src/container/NewDashboard/DashboardVariablesSelection/VariableItem.tsx:242
  • Draft comment:
    Renaming the parameter to 'inputValue' and converting falsy multiSelect values to an empty array clarifies intent. Ensure that this check doesn’t inadvertently treat valid falsy values (if any) as empty selections.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
5. frontend/src/container/NewDashboard/DashboardVariablesSelection/VariableItem.tsx:23
  • Draft comment:
    Typographical error: 'dashbaordVariables' should be 'dashboardVariables'. Please update the import path accordingly.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. frontend/src/container/NewDashboard/DashboardVariablesSelection/VariableItem.tsx:24
  • Draft comment:
    Typographical error: 'dashbaordVariables' should be 'dashboardVariables' in the import statement. Please fix this typo.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_ApWlAnhrKtpzdm9s


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@SagarRajput-7 SagarRajput-7 merged commit e92e0b6 into main Mar 10, 2025
14 of 16 checks passed
@SagarRajput-7 SagarRajput-7 deleted the SIG-7243 branch March 10, 2025 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants