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: Allow to edit registries #198

Merged
merged 7 commits into from Jun 15, 2022
Merged

feat: Allow to edit registries #198

merged 7 commits into from Jun 15, 2022

Conversation

ankanroy-code
Copy link
Contributor

@ankanroy-code ankanroy-code commented Jun 13, 2022

UI - add feature : #156

make registries editable

Untitled.mp4

HOW I DID IT?
basic idea: delete the old registry create a new registry, using the old registry and the changed data.

PROBLEMS?

  • only when you change the URL in edit, it create a new registry, and dose not delete the old one.

Please let me know any changes you world like

@ankanroy-code ankanroy-code changed the title Editmode Edit registries Jun 13, 2022
@benoitf
Copy link
Collaborator

benoitf commented Jun 13, 2022

Hello @ankanroy-code and thanks for your contribution 🎉

To be able to merge pull requests on containers organization, commits need to be signed Off.
https://github.com/containers/podman-desktop/pull/198/checks?check_run_id=6863265316

@ankanroy-code
Copy link
Contributor Author

@benoitf thanks i did it.

@slemeur
Copy link
Collaborator

slemeur commented Jun 13, 2022

Thanks for your contribution @ankanroy-code !!

@benoitf
Copy link
Collaborator

benoitf commented Jun 13, 2022

Hello, you should probably do a rebase as well of the branch to the main branch.

image

it looks like there are commits in your PR that are already in the current main branch

@ankanroy-code
Copy link
Contributor Author

@benoitf hey i did a rebase of editmode to my main. did it work. i am new to git. so sorry,it taking time to understand what commands am i passing.

@benoitf
Copy link
Collaborator

benoitf commented Jun 13, 2022

@ankanroy-code it seems not. I mean, we shouldn't see the commits that are already in the main branch.

@ankanroy-code
Copy link
Contributor Author

@benoitf should i make a new PR from the main?

@benoitf
Copy link
Collaborator

benoitf commented Jun 13, 2022

@ankanroy-code changing git history can be done in your branch. No need to create a new one. But if it's easier for you, you may try with a new one.
If you're not able to rebase properly I could attempt to do it.

@benoitf
Copy link
Collaborator

benoitf commented Jun 13, 2022

@ankanroy-code also probably we would need to share some stuff between the 'creation' and the 'update' component

@ankanroy-code
Copy link
Contributor Author

@ankanroy-code changing git history can be done in your branch. No need to create a new one. But if it's easier for you, you may try with a new one. If you're not able to rebase properly I could attempt to do it.

@benoitf sorry for late reply, went to college today for the first time. Just came back. If you so the rebase, please do it, it would be great help.

@ankanroy-code
Copy link
Contributor Author

@ankanroy-code also probably we would need to share some stuff between the 'creation' and the 'update' component

Can you give me some example? Or a bit more specific explanation. I am new, so the terms are not clear. I googled it, but could not understand the question.

@benoitf
Copy link
Collaborator

benoitf commented Jun 14, 2022

Can you give me some example? Or a bit more specific explanation. I am new, so the terms are not clear. I googled it, but could not understand the question.

The idea is that by having a common component, if for example we add a new parameter to the 'creation' panel then it's handled by the 'update/edit' part

@benoitf
Copy link
Collaborator

benoitf commented Jun 14, 2022

FYI I did rebase your branch

$ git remote add upstream https://github.com/containers/podman-desktop
$ git fetch upstream
$ git pull --rebase upstream main
$ git push -f origin editmode

@ankanroy-code
Copy link
Contributor Author

@benoitf what is 'creation' panel and what is 'update/edit' part. If you can specify the code, it world be great.

@@ -3,6 +3,7 @@ import { onMount } from 'svelte';
import type { Registry } from '@tmpwip/extension-api';
import { registriesInfos } from '../../stores/registries';
import PreferencesRegistriesCreateRegistryModal from './PreferencesRegistriesCreateRegistryModal.svelte';
import PreferncesEditRegistrieModal from './PreferncesEditRegistrieModal.svelte';
Copy link
Collaborator

Choose a reason for hiding this comment

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

the filename has a typo (missing e in Preferences) and missing s at RegistrieS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI I did rebase your branch

$ git remote add upstream https://github.com/containers/podman-desktop
$ git fetch upstream
$ git pull --rebase upstream main
$ git push -f origin editmode

@benoitf hey I fixed it. Which brunch should I push my code to, Main or editmode.

Copy link
Collaborator

Choose a reason for hiding this comment

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

always push to editmode (this is a the branch used to create this Pull Request)

@ankanroy-code
Copy link
Contributor Author

FYI I did rebase your branch

$ git remote add upstream https://github.com/containers/podman-desktop
$ git fetch upstream
$ git pull --rebase upstream main
$ git push -f origin editmode

@benoitf thxxxxxxx

@benoitf
Copy link
Collaborator

benoitf commented Jun 14, 2022

There is a PR check that is failing (linter/formatter)

Ensure to run the command yarn format:fix && yarn lint:fix

@ankanroy-code
Copy link
Contributor Author

@benoitf I fixed the linter/formatter issue

@benoitf
Copy link
Collaborator

benoitf commented Jun 14, 2022

@ankanroy-code I don't see the modal file ( I see you removed the old file but in PR diff I don't see the new file)

@benoitf
Copy link
Collaborator

benoitf commented Jun 14, 2022

also I don't see any common code between create and edit modal that will lead to something harder to maintain

@ankanroy-code
Copy link
Contributor Author

also I don't see any common code between create and edit modal that will lead to something harder to maintain

Working on. It will be over in 4 hours

@ankanroy-code
Copy link
Contributor Author

@benoitf I made a common file for the functions that are reused

@benoitf
Copy link
Collaborator

benoitf commented Jun 15, 2022

I've added changes to your branch to use a single modal file (two mode: create or edit). It avoids to duplicate 200lines of code.
Also I've added update event (instead of having to perform remove and then add)

changes are there e458aa5

Signed-off-by: ankanroy-code <8ankanroy@gmail.com>
Signed-off-by: ankanroy-code <8ankanroy@gmail.com>
Signed-off-by: ankanroy-code <8ankanroy@gmail.com>
Signed-off-by: ankanroy-code <8ankanroy@gmail.com>
Signed-off-by: ankanroy-code <8ankanroy@gmail.com>
ankanroy-code and others added 2 commits June 15, 2022 09:29
Signed-off-by: ankanroy-code <8ankanroy@gmail.com>
Also add new event for update (instead of remove/add)

Change-Id: I4e221e6ad5508681972e560a94a3f37781750c72
Signed-off-by: Florent Benoit <fbenoit@redhat.com>
@ankanroy-code
Copy link
Contributor Author

I've added changes to your branch to use a single modal file (two mode: create or edit). It avoids to duplicate 200lines of code. Also I've added update event (instead of having to perform remove and then add)

changes are there e458aa5

@benoitf Nice, It reduced a lot of code. Also fixed all the problems. Thanks!!! For your help.

@benoitf benoitf merged commit 8c8955e into containers:main Jun 15, 2022
@benoitf benoitf changed the title Edit registries feat: Allow to edit registries Jun 15, 2022
@podman-desktop-bot podman-desktop-bot added this to the 0.0.5 milestone Jun 15, 2022
@benoitf benoitf mentioned this pull request Jun 22, 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

4 participants