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

Added confirmation dialog for "Distrust System Certificates" #1307

Open
wants to merge 4 commits into
base: main-ose
Choose a base branch
from

Conversation

ArnyminerZ
Copy link
Member

Purpose

See #1294

Short description

  • Added a confirmation dialog for enabling the "Distrust System Certificates" option.
  • Disabling the option doesn't show any warning.

Checklist

  • The PR has a proper title, description and label.
  • I have self-reviewed the PR.
  • I have added documentation to complex functions and functions that can be used by other modules.
  • I have added reasonable tests or consciously decided to not add tests.

@ArnyminerZ ArnyminerZ linked an issue Feb 11, 2025 that may be closed by this pull request
@ArnyminerZ ArnyminerZ self-assigned this Feb 11, 2025
@ArnyminerZ ArnyminerZ added the enhancement New feature or request label Feb 11, 2025
@ArnyminerZ
Copy link
Member Author

image

Signed-off-by: Arnau Mora <[email protected]>
@ArnyminerZ ArnyminerZ marked this pull request as ready for review February 11, 2025 19:05
@ArnyminerZ ArnyminerZ changed the title Added confirmation dialog for dsc Added confirmation dialog for "Distrust System Certificates" Feb 11, 2025
Copy link
Member

@sunkup sunkup left a comment

Choose a reason for hiding this comment

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

Works well :)

I would probably write something different, though - but we can also stick to your version. The most important is to have the user understand they are doing some potentially functionality breaking changes.

Title: "Attention!"
Text: "Enabling this option will cause DAVx5 to distrust system certificates. You will need to manually install the required certificates. This option is intended for advanced users."

Screenshot

image

What do you think? Also how about a preview of the card?

app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@ArnyminerZ
Copy link
Member Author

Added preview:

Image

image

@ArnyminerZ ArnyminerZ requested a review from sunkup February 13, 2025 08:51
@ArnyminerZ
Copy link
Member Author

I've also updated the text as you've said. @devvv4ever anything to change? See screenshot above

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

Successfully merging this pull request may close these issues.

[UI] Too easy to activate "Distrust system certs"
2 participants