-
Notifications
You must be signed in to change notification settings - Fork 30
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
Add detailed diffs of changes made to secrets #274
Comments
hey, I would like to work on this issue. @rohan-chaturvedi |
Great, its all yours! |
![]() ![]() Hey @rohan-chaturvedi just wanted a little clarification about the UI, for testing I have created a dropdown ui, should I make a separate pop up screen/page for the changes or should I just try to make the current dropdown look cleaner? |
Hey @harshith-1008 , this is a good start! However I think we should avoid this "accordion" style behavior, since it shifts the entire rest of the UI down. Instead, how about trying a modal dialog? We have a |
thanks for the feedback, ill try to implement it. |
@rohan-chaturvedi I tried using genericDialogue component, but it expects button variant props, if I pass button variant then the alert changes to green box around the text. I have changed the component such that it accepts an empty string as props for genericDialogue, is it fine? |
@harshith-1008 that should be fine, just make sure that its not a breaking change for other components that use it. I'll take a look in detail once you have a PR. Once thing you could try is adding an |
![]() ![]() @rohan-chaturvedi I hope this UI is fine. |
This is a great start! A few suggestions:
|
![]() @rohan-chaturvedi is this ui okay? |
That looks awesome! A few notes:
You could use the secret history UI as a baseline for this as well: |
@harshith-1008 The SecretPropertyDiffs component could be useful for rendering details of changes |
@rohan-chaturvedi just one small clarification, the font-mono should be just on keys name right? not the properties name right? |
Yep, I think that will work well |
![]() @rohan-chaturvedi Is this UI fine? |
@harshith-1008 This is good progress! The UI still needs a bit of work though. Could you tag me on the PR when its ready? It will be easier to suggest changes to the styles. |
* Secret changes dialog added * added license * Update and rename LICENSE.txt to LICENSE * figma design changes * fixed font irregularities * fixed font * renamed getclientSecrets to its original name * Created DeployPreview component --------- Co-authored-by: Nimish <85357445+nimish-ks@users.noreply.github.com> Co-authored-by: Rohan Chaturvedi <rohan.chaturvedi@protonmail.com>
Is your feature request related to a problem?
Making changes to secrets currently shows a "You have undeployed changes to this environment." warning banner and an amber highlight to affected secrets, but it can be difficult to determine what changes were made other than which secrets are affected.
2024-06-16.20-44-40.webm
Describe the solution you'd like
Clicking the warning banner should show a modal with details of secrets affected, with diffs of specific properties changed, such as tags, comments, etc. For example:
GOOGLE_CLIENT_ID
TEST
SESSION_COOKIE_DOMAIN
Ideally, something inspired by VSCode's diffs would be great:
The text was updated successfully, but these errors were encountered: