8000 Add Confirmation Dialog on Device Type Feature Toggle and Disable Toggling when Related Cluster is Disabled by ethanzhouyc · Pull Request #1554 · project-chip/zap · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add Confirmation Dialog on Device Type Feature Toggle and Disable Toggling when Related Cluster is Disabled #1554

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
May 20, 2025
Merged

Add Confirmation Dialog on Device Type Feature Toggle and Disable Toggling when Related Cluster is Disabled #1554

merged 3 commits into from
May 20, 2025

Conversation

ethanzhouyc
Copy link
Collaborator
@ethanzhouyc ethanzhouyc commented Mar 31, 2025 •
edited
Loading
  • Added a confirmation dialog when user toggles a device type feature. After viewing the list of elements to be updated, user can choose to proceed with the change or cancel it.
  • Disabled toggling of a device type feature if its associated cluster is disabled, and displayed warnings to inform user which cluster is missing.

ZAPP-1549, ZAPP-1552

@ethanzhouyc ethanzhouyc requested a review from dhchandw March 31, 2025 20:34
@paulr34
Copy link
Collaborator
paulr34 commented Mar 31, 2025

It would be smooth if this happened once and not every single time, what do you think @ethanzhouyc @tecimovic @brdandu ?

ethanzhouyc
ethanzhouyc commented Mar 31, 2025
View reviewed changes
Copy link
Collaborator Author
@ethanzhouyc ethanzhouyc left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Confirmation dialog on feature toggle:
image

Disable toggling and display warnings for features with disabled cluster.
image

@paulr34
Copy link
Collaborator
paulr34 commented Mar 31, 2025

Confirmation dialog on feature toggle:

image

Disable toggling and display warnings for features with disabled cluster.

image

The user could just disable the feature and get the same results right?

@ethanzhouyc
Copy link
Collaborator Author
ethanzhouyc commented Mar 31, 2025 •
edited
Loading

@paulr34 I think this confirmation should happen on every feature toggle to ensure ZAP doesn’t notify users about changes they didn’t want but were already committed. Elements updated among different feature toggles can vary a lot.
I am planning to set a meeting for getting feedback from the team after completing all features and we could discuss this then.

@paulr34
Copy link
Collaborator
paulr34 commented Mar 31, 2025

It would be cool if there was a button that said "don't show me this again" which is a pretty standard practice

@ethanzhouyc
Copy link
Collaborator Author

@paulr34 When user enables and then disables a feature, ZAP might "correct" some elements to align with the expected conformance. While this is technically correct, it could still modify user configurations that they didn't intend to change. By allowing the user to cancel the change, we ensure ZAP doesn't modify anything without explicit confirmation.

@paulr34
Copy link
Collaborator
paulr34 commented Mar 31, 2025

@paulr34 When user enables and then disables a feature, ZAP might "correct" some elements to align with the expected conformance. While this is technically correct, it could still modify user configurations that they didn't intend to change. By allowing the user to cancel the change, we ensure ZAP doesn't modify anything without explicit confirmation.

Sure that makes sense. It would be great if I could opt out by clicking "don't show me this again" but otherwise it's great good job

@paulr34
Copy link
Collaborator
paulr34 commented Mar 31, 2025

@paulr34 When user enables and then disables a feature, ZAP might "correct" some elements to align with the expected conformance. While this is technically correct, it could still modify user configurations that they didn't intend to change. By allowing the user to cancel the change, we ensure ZAP doesn't modify anything without explicit confirmation.

Sure that makes sense. It would be great if I could opt out by clicking "don't show me this again" but otherwise it's great good job

Correction: "don't ask me this again" if you want to show what happened

@ethanzhouyc
Copy link
Collaborator Author

That's a cool idea! I take it down and will bring it up when discussing feedback with team.

@ethanzhouyc
Copy link
Collaborator Author

Issue #1563 created to add Cypress tests covering the feature page related code in future PR

brdandu
brdandu approved these changes Apr 3, 2025
View reviewed changes
Copy link
Collaborator
@brdandu brdandu left a comment

Choose a reason for hiding this comment

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

Just fix the comments. Looks good!

@ethanzhouyc
Copy link
Collaborator Author

@paulr34 #1568 Issue created on the 'Don't shown me again' option and will discuss before merge

@ethanzhouyc
Copy link
Collaborator Author

Discussed with team and the confirmation dialog should be displayed on each toggle.

@ethanzhouyc ethanzhouyc merged commit 22a7ca3 into project-chip:master May 20, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brdandu brdandu brdandu approved these changes

@paulr34 paulr34 paulr34 approved these changes

@tecimovic tecimovic Awaiting requested review from tecimovic tecimovic is a code owner

@dhchandw dhchandw Awaiting requested review from dhchandw

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ethanzhouyc @paulr34 @brdandu
0