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

feat: update registries editing page #747

Merged
merged 1 commit into from
Nov 23, 2022
Merged

feat: update registries editing page #747

merged 1 commit into from
Nov 23, 2022

Conversation

vzhukovs
Copy link
Contributor

@vzhukovs vzhukovs commented Nov 2, 2022

What does this PR do?

Part of #446

Provides an updated UI page for editing registries.

Signed-off-by: Vladyslav Zhukovskyi vzhukovs@redhat.com

Screenshot/screencast of this PR

Снимок экрана 2022-11-02 в 11 51 18

What issues does this PR fix or reference?

#446

How to test this PR?

Open the Registries page in Settings menu.

@vzhukovs vzhukovs self-assigned this Nov 2, 2022
@benoitf benoitf changed the title refactor: update registries editing page feat: update registries editing page Nov 2, 2022
@benoitf
Copy link
Collaborator

benoitf commented Nov 3, 2022

It looks like the input fields are not reusing the colors on the other screens
image

vs
image

@benoitf
Copy link
Collaborator

benoitf commented Nov 3, 2022

I'm unsure that the menu should appear when I hover the kebab menu. I expect to click on the button to see the context ?

j31BVEkTnP.mp4

@vzhukovs
Copy link
Contributor Author

Updated PR by reusing the kebab menu from the following PR #804

Not the popup menu looks like this:

Снимок экрана 2022-11-15 в 18 20 49

cc @benoitf @cdrage

@vzhukovs vzhukovs force-pushed the pd#446 branch 3 times, most recently from cf5f5b4 to 7c2d611 Compare November 21, 2022 12:47
Comment on lines 273 to 275
!serviceUrl.match(
/(http(s)?:\/\/.)?(www\.)?[-a-zA-Z0-9@:%._+~#=]{2,256}\.[a-z]{2,6}\b([-a-zA-Z0-9@:%_+.~#?&//=]*)/g,
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

it looks this is a wrongly regexp #835
fixed by #838

so probably need to use the same validator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved dev dependency to the upper level to be accessible in main package and updated validation to use this valiadtor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but I'm not sure, should we also mention this library in vite.config.js here https://github.com/containers/podman-desktop/blob/pd%23446/packages/main/vite.config.js#L50

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it works in production mode, no need. If it doesn't it needs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it, it works in production mode. auxiliary, I've added types for validation package, just to avoid warns that any type is used in runtime


modifiedRegistries.push(modifiedRegistry);

updateUI();
Copy link
Collaborator

Choose a reason for hiding this comment

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

hello, why do we have these updateUI() calls ?

UI should refresh automatically

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it should, but it actually doesn't refresh when we iterate over storage elements and need dynamic change the dom elements on the UI after processing onClick events, updating storage with self elements solves this problem and I think it's like a bug in svelte. so yes, here is a little workaround

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we're not doing something correct on our side.

For example for svelte, we need to reassign a value, not append to an array for example

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess you should do

 modifiedRegistries = [...modifiedRegistries, modifiedRegistry]

https://svelte.dev/tutorial/updating-arrays-and-objects

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated, thanks for pointing to this doc

@vzhukovs vzhukovs force-pushed the pd#446 branch 2 times, most recently from 523b011 to b0876e4 Compare November 22, 2022 16:44
@benoitf
Copy link
Collaborator

benoitf commented Nov 22, 2022

The code looks way better now. Thanks

I think the only blocker issue before merging is that the password is displayed by default when we add a registry or when clicking on edit.
it should be hidden by default and only be visible if user request it.

@vzhukovs
Copy link
Contributor Author

Снимок экрана 2022-11-22 в 20 23 55

added ability to toggle the password visibility

@vzhukovs vzhukovs force-pushed the pd#446 branch 3 times, most recently from 422f415 to b4760fc Compare November 22, 2022 21:43
@vzhukovs
Copy link
Contributor Author

latest update with the UI:
Снимок экрана 2022-11-22 в 23 42 37

Signed-off-by: Vladyslav Zhukovskyi <vzhukovs@redhat.com>
Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

checked with all latest enhancements and it works great now 🎉

@vzhukovs vzhukovs merged commit c462c96 into main Nov 23, 2022
@vzhukovs vzhukovs deleted the pd#446 branch November 23, 2022 08:56
@podman-desktop-bot podman-desktop-bot added this to the 0.10.0 milestone Nov 23, 2022
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.

None yet

3 participants