Skip to content

Fix disable summaries sample logic#3922

Open
MarielaTihova wants to merge 1 commit intovnextfrom
mtihova/fix-3892
Open

Fix disable summaries sample logic#3922
MarielaTihova wants to merge 1 commit intovnextfrom
mtihova/fix-3892

Conversation

@MarielaTihova
Copy link
Contributor

Resolves #3892

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes an inverted logic bug in the Grid Disable Summaries sample component, as reported in issue #3892. Previously, selecting a checkbox in the dropdown would hide a summary, making the UX counterintuitive. Now, checking a checkbox shows a summary (enabling it), and unchecking it hides (disables) it — aligning with standard user expectations.

Changes:

  • Inverted the checkbox state logic so checked: true means "summary is enabled/visible" and unchecking adds it to disabledSummaries.
  • Swapped the "Disable All" and "Enable All" button bindings and their respective disabled-state conditions.
  • Updated the dropdown panel heading from "Disable Summaries for Column:" to "Toggle Summaries for Column:" to better reflect the new behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
grid-disable-summaries.component.ts Inverts checked initial value, toggleCheckbox condition, and swaps uncheckAllColumns/checkAllColumns implementations
grid-disable-summaries.component.html Swaps button-to-handler bindings and disabled conditions for "Disable All"/"Enable All"; updates section title

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copy link
Contributor

@georgianastasov georgianastasov left a comment

Choose a reason for hiding this comment

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

For now I reject this pull request and it should not be merged without the opinion of the UI/UX person, more precisely without the opinion of @sdimchevski and @yradoeva because they are the people who created the sample design. We have a lot of discussions about that when the feature and sample were implemented and the final decision was that behavior: "all checkboxes enabled at the beginning and checking disables the summary, this is what most corresponds to the name of the property and what we want to show".

@MarielaTihova
Copy link
Contributor Author

For now I reject this pull request and it should not be merged without the opinion of the UI/UX person, more precisely without the opinion of @sdimchevski and @yradoeva because they are the people who created the sample design. We have a lot of discussions about that when the feature and sample were implemented and the final decision was that behavior: "all checkboxes enabled at the beginning and checking disables the summary, this is what most corresponds to the name of the property and what we want to show".

In my opinion the name should be changed. It is in no way intuitive to have checkboxes checked when something is disabled and unchecked checkboxes when something is enabled. It was confusing for me as a developer. I imagine it would be even more confusing for the users. I accept feedback from @sdimchevski and @yradoeva of course.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants