-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
feat(data-secrecy): Data Secrecy Settings UI #75791
Conversation
Bundle ReportChanges will increase total bundle size by 6.22kB ⬆️
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thoughts about the UI:
can we group the data secrecy toggles in one section? or move the waiver section right underneath the main toggle?
grouping is what i was going for, but the form component we have is very finicky where it only lets us add 1 endpoint for it to call to update values when we use onBlur to update different settings individually. i don't want to refactor the component cause it's used by majority of the UI and I don't have the context for it. Let me look into moving it tho |
i moved the section! |
Major pieces are in place. I can help to prettify this on Fri. |
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #75791 +/- ##
==========================================
+ Coverage 78.15% 78.23% +0.07%
==========================================
Files 6910 6899 -11
Lines 307183 306450 -733
Branches 50351 50233 -118
==========================================
- Hits 240080 239749 -331
+ Misses 60754 60309 -445
- Partials 6349 6392 +43 |
fixed the issue Screen.Recording.2024-08-25.at.5.40.31.PM.mov |
what does it look like when you have an access period in the past and you open the page? should we render an info message for that and/or when you don't have access to toggle it? |
When the toggle is off and the time is in the past my suggestion is to clear the time field, showing basically there's no access going on at the moment. |
[`/organizations/${organization.slug}/data-secrecy/`], | ||
{ | ||
staleTime: 3000, | ||
retry: (failureCount, error) => failureCount < 3 && error.status !== 404, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we retry? I just don't see this often in our frontend code so wondering how this endpoint is different from others that needs retrying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
most endpoints are able to use the default retry, but since the api will return a 404 if an org doesn't have a waiver currently, the default behavior of the api would be to keep refreshing then render a failure state. here we prevent this by not refreshing on 404s.
i agree with when the toggle is off, but my concern with hiding when its in the past is we don't currently show when it was past easily from the UI, so i could see someone waiving then coming back and being confused why it disappeared and think its a bug. |
It's fine as is. I don't feel strongly for it. |
Here I add the settings for Data Secrecy in the frontend. There are two:
Updated Demo: #75791 (comment)
Old Demo
Screen.Recording.2024-08-07.at.1.28.31.PM.mov
Screen.Recording.2024-08-07.at.2.45.14.PM.mov
Rewatch Links for Demo:
https://sentry.rewatch.com/video/ispnhixepw165zl9-screen-recording-2024-08-07-at-1-28-31-pm
https://sentry.rewatch.com/video/2jucpfwxswxs0dv6-screen-recording-2024-08-07-at-2-45-14-pm
Update:
I moved the waive setting so that its under the original toggle