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

enh: warn during table manager promotion/demotion #1434

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

Conversation

enjeck
Copy link
Contributor

@enjeck enjeck commented Oct 29, 2024

No description provided.

@enjeck enjeck self-assigned this Oct 29, 2024
@enjeck enjeck force-pushed the warn-table-manager branch from 8cf9e5c to 74f35ef Compare December 4, 2024 09:55
@enjeck enjeck changed the title enh: warn about table use in app during manager demotion enh: warn during table manager promotion/demotion Dec 4, 2024
@enjeck enjeck force-pushed the warn-table-manager branch from 74f35ef to 78bbc66 Compare December 4, 2024 09:56
@enjeck enjeck marked this pull request as ready for review December 4, 2024 09:57
@enjeck enjeck requested a review from blizzz as a code owner December 4, 2024 09:57
@enjeck enjeck requested a review from juliusknorr December 4, 2024 09:57
@enjeck enjeck added enhancement New feature or request 3. to review Waiting for reviews labels Dec 4, 2024
@enjeck
Copy link
Contributor Author

enjeck commented Dec 4, 2024

Now, there's a confirmation screen that warns about tables when a user attempt to promote to a manager:
Screenshot from 2024-11-30 09-54-54

There's also a warning display after demotion. Would like to hear what @nextcloud/designers think :)

Initially, I attempted to fetch all Applications and check if the table is in use (so that we only show the warning if there table is actually used in some Application), but that seemed to be getting to complicated. Pushed unfinished code to main...warn-demote-check-apps

Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Looks good and wording also fits from my perspective 👍

Copy link
Member

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

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

I think it would be nice to have a "cancel" button too?
And is this a destructive action? We usually reserve the color error for such actions

@enjeck
Copy link
Contributor Author

enjeck commented Dec 10, 2024

And is this a destructive action? We usually reserve the color error for such actions

Not really. I can change to the 'warning' color then

@enjeck enjeck force-pushed the warn-table-manager branch from 807390d to d7785c5 Compare December 13, 2024 16:09
@enjeck
Copy link
Contributor Author

enjeck commented Dec 13, 2024

Now looks like so:

Screenshot from 2024-12-13 15-06-25

@marcoambrosini
Copy link
Member

@enjeck looks good! let's just right align the cancel button like in other dialogs and we're golden.

Signed-off-by: Cleopatra Enjeck M. <patrathewhiz@gmail.com>
Signed-off-by: Cleopatra Enjeck M. <patrathewhiz@gmail.com>
@enjeck enjeck force-pushed the warn-table-manager branch from d7785c5 to a259596 Compare December 16, 2024 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature or request
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

3 participants