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

Add confirmation on container remove #61

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

alessandrodolci
Copy link
Contributor

@alessandrodolci alessandrodolci commented Jul 11, 2020

Hello Guillaume,

here I am with the implementation of the confirmation dialog for the remove action.

I introduced a reusable component in a separate file, inheriting from Gnome Shell base ModalDialog, and added a new property to the actions dictionary, indicating whether an action needs the user to confirm for its execution.
I also started worrying a bit about the hierarchy of source files at this point, but haven't had any change yet. I thought that maybe it could be reorganized by placing UI components in a dedicated folder, like this:

- src
    - ui
        - confirmationModal.js
        - dockerMenu.js
        - dockerMenuItem.js
        - dockerMenuStatusItem.js
        - dockerSubMenuMenuItem.js
    - docker.js
    - utils.js

I tested it against gnome-shell 3.36.3 on Ubuntu 20.04 and 3.28.4 on Ubuntu 18.04, and haven't experienced any particular issue.

As always, consider the implementation as a draft, and please feel free to propose changes if you don't like something, you know I don't want to overcome your voice on structure decisions for the project!

Have a good weekend,
Alessandro

@alessandrodolci
Copy link
Contributor Author

Hello @gpouilloux,

any update on this?
Don't want to put any pressure, but it's been a while since I've heard from you, I hope everything's all right!
Let me know if I can help with something, I'm always willing to keep working on this.

Alessandro

@gpouilloux
Copy link
Owner

Hi @alessandrodolci

Sorry, I've had little time to consider to this project.
The implementation looks good to me, well done on this!

@alessandrodolci
Copy link
Contributor Author

Hi!

That's very reasonable, I hope I haven't bothered you!
Anyway, good, thank you for the feedback! I'll try to have a look on the remaining issues if I'll have the time.

Have a nice day,
Alessandro

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.

2 participants