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

Do not hide not secret settings in the web plugin UI #1964

Merged
merged 6 commits into from
Jun 5, 2023
Merged

Do not hide not secret settings in the web plugin UI #1964

merged 6 commits into from
Jun 5, 2023

Conversation

alexintech
Copy link
Contributor

@alexintech alexintech commented May 18, 2023

What this PR does

In the web plugin UI Settings -> Env Variables if variable is not secret, do not hide its value.

Which issue(s) this PR fixes

Checklist

  • Unit, integration, and e2e (if applicable) tests updated
  • Documentation added (or pr:no public docs PR label added if not required)
  • CHANGELOG.md updated (or pr:no changelog PR label added if not required)

@alexintech alexintech requested a review from a team May 18, 2023 08:26
@Konstantinov-Innokentii
Copy link
Member

@vadimkerr what do you think?

@vstpme vstpme added the pr:no public docs Added to a PR that does not require public documentation updates label May 22, 2023
@vstpme
Copy link
Member

vstpme commented May 22, 2023

I personally like all values hidden by default so it's safe to open this page when screen sharing / making screenshots. Hiding all values by default also makes things simpler IMO, so there's only two options – either all settings are hidden or all settings are exposed.
wdyt @grafana/grafana-oncall-frontend @Matvey-Kuk?

@vstpme vstpme requested a review from Matvey-Kuk May 22, 2023 20:04
@Konstantinov-Innokentii
Copy link
Member

@vadimkerr I don't like, that all values are hidden. With ability to change phone provider or ESP for inbound email it feels strange to hide chosen options.

@Matvey-Kuk Matvey-Kuk requested review from vstpme and removed request for Matvey-Kuk May 29, 2023 09:58
@alexintech
Copy link
Contributor Author

When I want to change the value of a setting, I scroll down for some non-secret setting. But when I press the "Edit" button, the input is cleared, so you can't see the current value, because of this <Text> behavior.

setValue(clearBeforeEdit || hidden ? '' : (children as string));

I close the modal window, scroll up, click the "Show values" button, scroll down again, and only then can I edit the value. After refreshing the page, all the values are hidden again. That's a little cumbersome.

CHANGELOG.md Outdated Show resolved Hide resolved
alexintech and others added 2 commits May 31, 2023 19:12
Co-authored-by: Joey Orlando <joseph.t.orlando@gmail.com>
@alexintech alexintech requested a review from joeyorlando May 31, 2023 13:19
Copy link
Contributor

@joeyorlando joeyorlando left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Consider adding a jest snapshot test for the two cases where a value is hidden, and when it is not hidden.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit can the hideValues && item.is_secret logic be deduplicated into a function on the LiveSettings class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't suppose that it's necessary.

@alexintech
Copy link
Contributor Author

Overall LGTM. Consider adding a jest snapshot test for the two cases where a value is hidden, and when it is not hidden.

I've tried to write a test, but I have not much experience in frontend. All my attempts failed.

@vstpme vstpme enabled auto-merge June 5, 2023 12:21
@vstpme vstpme added this pull request to the merge queue Jun 5, 2023
Merged via the queue into grafana:dev with commit 5f067af Jun 5, 2023
@alexintech alexintech deleted the hide-only-secret branch June 5, 2023 12:35
brojd pushed a commit that referenced this pull request Sep 18, 2024
# What this PR does

In the web plugin UI Settings -> Env Variables if variable is not
secret, do not hide its value.

## Which issue(s) this PR fixes

## Checklist

- [ ] Unit, integration, and e2e (if applicable) tests updated
- [ ] Documentation added (or `pr:no public docs` PR label added if not
required)
- [x] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not
required)

---------

Co-authored-by: Joey Orlando <joseph.t.orlando@gmail.com>
Co-authored-by: Joey Orlando <joey.orlando@grafana.com>
Co-authored-by: Vadim Stepanov <vadimkerr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:no public docs Added to a PR that does not require public documentation updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants