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

Warn the user about unsaved changes (services selection) #96

Merged
merged 2 commits into from
Oct 9, 2018

Conversation

teclator
Copy link
Contributor

@teclator teclator commented Oct 9, 2018

@coveralls
Copy link

coveralls commented Oct 9, 2018

Coverage Status

Coverage increased (+0.08%) to 17.54% when pulling e78366d on services_validation into 73cb27e on SLE-15-GA.

Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

In general it looks good. Please, fix the changelog.

Tue Oct 9 08:55:56 UTC 2018 - knut.anderssen@suse.com

- Alert the user about unsaved changes when leaving the zone
services configuration without aplying changes (fate#324662)
Copy link
Contributor

Choose a reason for hiding this comment

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

applying

@imobachgs
Copy link
Contributor

np: I am not sure about the term "applying".

@mvidner
Copy link
Member

mvidner commented Oct 9, 2018

IMHO people will confuse the "applying" with "accepting" the whole module. I would call it "adding", like the button that does it.

Also, we are giving 2 choices:
1: Yes = Switch tabs, discarding the selected services
2: No = Don't switch tabs

But most likely, the user wants to do something different:
3: Switch tabs and Add the services to the Allowed set beforehand.

So I would like to have a choice between 3 and 1, omitting 2 entirely:

The selected services have not yet been added to the allowed set
[Add] [Discard]

@teclator
Copy link
Contributor Author

teclator commented Oct 9, 2018

IMHO people will confuse the "applying" with "accepting" the whole module. I would call it "adding", like the button that does it.

That is I why I specified "the selection of services", and it is for both actions ("adding" or "removing" services")

Also, we are giving 2 choices:
1: Yes = Switch tabs, discarding the selected services
2: No = Don't switch tabs

But most likely, the user wants to do something different:
3: Switch tabs and Add the services to the Allowed set beforehand.

Yep, I was thinking on this option also, but discarded for not complicate it more.

So I would like to have a choice between 3 and 1, omitting 2 entirely:

The selected services have not yet been added to the allowed set
[Add] [Discard]

In this case, [Apply Changes][Discard] ?

@mvidner
Copy link
Member

mvidner commented Oct 9, 2018

Ah, you mean [Apply Changes] means to Add the ones in the left column and Remove those in the right column, right? I missed that because the screenshot does not show that complex case.

Well, then [Apply Changes][Discard] is good enough. (We could make it clearer but the code would get too complicated)

@teclator
Copy link
Contributor Author

teclator commented Oct 9, 2018

Ah, you mean [Apply Changes] means to Add the ones in the left column and Remove those in the right column, right? I missed that because the screenshot does not show that complex case.

Yep, I realized that and that is why I have updated the screenshot, hope it is more clear now.

Well, then [Apply Changes][Discard] is good enough. (We could make it clearer but the code would get too complicated)

Exactly, and in this case I am not sure if we should summarize which services will be added and which ones will be removed.

@teclator
Copy link
Contributor Author

teclator commented Oct 9, 2018

It seems that [Apply Changes] could be even a nive improvement, as currently you are only allowed to submit one of the list changes (Add or Remove)

@imobachgs
Copy link
Contributor

It seems that [Apply Changes] could be even a nive improvement, as currently you are only allowed to submit one of the list changes (Add or Remove)

Take into account that we took inspiration from the partitioner, so we should keep the same approach (to keep consistency).

@teclator
Copy link
Contributor Author

teclator commented Oct 9, 2018

It seems that [Apply Changes] could be even a nive improvement, as currently you are only allowed to submit one of the list changes (Add or Remove)

Take into account that we took inspiration from the partitioner, so we should keep the same approach (to keep consistency).

So, what do you suggest?

@teclator
Copy link
Contributor Author

teclator commented Oct 9, 2018

@imobachgs @mvidner mmm, what about leaving it as it is until we get some feedback from @kwwii

@imobachgs
Copy link
Contributor

So, what do you suggest?

To be honest, I do not have a better alternative. I only mentioned that the "apply" term could be confusing.

@teclator teclator merged commit 593a1e2 into SLE-15-GA Oct 9, 2018
@teclator teclator deleted the services_validation branch October 9, 2018 21:37
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.

4 participants