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

Auto capitalization toggle #292

Merged
merged 9 commits into from
Feb 8, 2023

Conversation

HasanMansoor4
Copy link
Contributor

Description 📣

New feature related to issue #276 Allow the user to toggle off/on the Auto Capitalization of secrets keys, by having a checkbox in the settings. I would love some feedback as this is my very first open source contribution. Thank you!

Screenhot

Feb-03-2023 04-51-47

@HasanMansoor4 HasanMansoor4 changed the title Auto capitalization toggle #276 Auto capitalization toggle Feb 3, 2023
@vmatsiiako
Copy link
Collaborator

vmatsiiako commented Feb 4, 2023

@HasanMansoor4 I think this is awesome!! It makes a lot of sense! Just two minor things:

  1. We moved settings to a new structure (it is now componerized across different files). Do you think you would be able to update it? If not, I could also try to do that :) Let me know!
  2. I assume default: true is going to make it work for old users too right? @dangtony98 would you be able to have a look at the backend? I think you have much more experience, so would love to hear if you think everything's ok

@vmatsiiako vmatsiiako requested a review from dangtony98 February 4, 2023 21:03
@HasanMansoor4
Copy link
Contributor Author

  1. We moved settings to a new structure (it is now componerized across different files). Do you think you would be able to update it? If not, I could also try to do that :) Let me know!

Yeah I just noticed the merge conflicts just now lol. I'm not that experienced with React/Next.js tbh I'll look into it and keep you updated.

  1. I assume default: true is going to make it work for old users too right? @dangtony98 would you be able to have a look at the backend? I think you have much more experience, so would love to hear if you think everything's ok

The auto capitalization boolean isn't backfilled for previous documents, but im using the ?? operator to default to true for undefined values. is this okay?

I'd love some feedback from @dangtony98

@HasanMansoor4
Copy link
Contributor Author

Hi @mv-turtle and @dangtony98, so I got the component refactored and its now using react query, I also removed the save button as I don't think there's a need for it. Would love some reviews. Thank you!

Feb-05-2023 06-31-17

@dangtony98
Copy link
Collaborator

Hey @HasanMansoor4!

Sorry for taking some time to review this ... I've left 2 comments on the backend side of things; just some small endpoint-naming change and fixing spelling for the controller function.

Other than that, it looks great to me!

@HasanMansoor4
Copy link
Contributor Author

Thank you @dangtony98. Yeah all your comments make sense, I have them fixed now.

@dangtony98
Copy link
Collaborator

@mv-turtle Are you good with this PR with regards to frontend?

I'm good on my end.

Copy link
Collaborator

@vmatsiiako vmatsiiako left a comment

Choose a reason for hiding this comment

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

This looks really great! Thank you so much for contributing @HasanMansoor4 :)

@vmatsiiako vmatsiiako merged commit 0d2cadd into Infisical:main Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants