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

frontend: Add secrets store page #1659

Merged
merged 23 commits into from
Mar 20, 2025

Conversation

malinskibeniamin
Copy link
Contributor

@malinskibeniamin malinskibeniamin commented Mar 17, 2025

@malinskibeniamin malinskibeniamin added feature New feature or request frontend ui/ux labels Mar 17, 2025
@malinskibeniamin malinskibeniamin self-assigned this Mar 17, 2025
@malinskibeniamin malinskibeniamin force-pushed the add-secret-management-page branch 7 times, most recently from 7438d7a to 2a6aaad Compare March 20, 2025 13:30
@malinskibeniamin malinskibeniamin force-pushed the add-secret-management-page branch from 2a6aaad to 63d7205 Compare March 20, 2025 13:30
@malinskibeniamin malinskibeniamin force-pushed the add-secret-management-page branch from c153ed6 to 6263e03 Compare March 20, 2025 15:53
Copy link
Collaborator

@jvorcak jvorcak left a comment

Choose a reason for hiding this comment

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

I'll check the rest later, for now I've put some preliminary review.

const [showValue, setShowValue] = useBoolean(false);

return (
<FormControl isInvalid={!!field.state.meta.errors?.length}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I though we had a PasswordInput component in the existing UI lib. Since we're using components from that library withing the file, could we use it as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's use this for now, the reason being is that this is a new component that uses tanstack form, I don't want to mix it up with the old implementation from component library. Also when using AI LLMs it doesn't work well with UI library because it does not have enough context of our internal API.


// Hack for MobX to ensure we don't need to use observables
export const updatePageTitle = () => {
runInAction(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I'd still prefer to stay consistent with the upper-level mobx classes to handle the logic and remove it with MobX later on. I think we would benefit from consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, I am trying to move quite fast, so I had to resort to this to ensure the title is set properly, otherwise it was overriding it.

The problem I have is that with MobX decorators and class-based components or observer pattern I will never get out of the legacy/tech debt we have today, so I decided this is the best we can do for now.

This is required because otherwise tests won't match the style guide
@malinskibeniamin malinskibeniamin force-pushed the add-secret-management-page branch from ed2c556 to 2bdd7de Compare March 20, 2025 17:56
@malinskibeniamin malinskibeniamin force-pushed the add-secret-management-page branch 2 times, most recently from c236493 to 3755ab5 Compare March 20, 2025 18:54
@malinskibeniamin malinskibeniamin force-pushed the add-secret-management-page branch from 3755ab5 to 4ef2c77 Compare March 20, 2025 19:17
@malinskibeniamin malinskibeniamin force-pushed the add-secret-management-page branch from 4ef2c77 to 4fa3410 Compare March 20, 2025 19:24
@malinskibeniamin malinskibeniamin merged commit fc7b83f into release-2.8 Mar 20, 2025
8 checks passed
@malinskibeniamin malinskibeniamin deleted the add-secret-management-page branch March 20, 2025 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request frontend ui/ux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants