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

Guided setup improvements #204

Merged
merged 3 commits into from Apr 12, 2017
Merged

Conversation

joseivanlopez
Copy link
Contributor

Add some improvements suggested by UX expert.

  • Skip first dialog when there is only one candidate disk.
  • Align widgets.

PBI: https://trello.com/c/crhdffWC/516-5-storageng-guided-setup

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 85.148% when pulling e54f1fb on joseivanlopez:guided_setup into d549d5a on yast:master.

return @candidate_disks if @candidate_disks
candidates = settings.candidate_devices || []
candidates = candidates.map { |d| analyzer.device_by_name(d) }
candidates = analyzer.candidate_disks if candidates.empty?
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if I get logic there. If settings is set, it will use it. And if settings is not set, it will return result of analyzer. So if you going back in guided setup and you previously pick only one disk, it will skip selecting disks, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, and that is wrong. I guess that it should always be considered as list of candidates (that is, the list of disks that could be selected) the result of the analyzer.


def expect_not_run_select_disks
expect_not_run_dialog(Y2Storage::Dialogs::GuidedSetup::SelectDisks)
end
Copy link
Member

Choose a reason for hiding this comment

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

what is goal of this shorthand. Expect is usually used once for specific test. Allow which is there before make sense, as allow is often used in before handlers and so on. So how often is this expect used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now only once, but maybe in next iterations we will skip more steps of the wizard. In these cases, we will expect to skip the same dialog several times.

Copy link
Member

Choose a reason for hiding this comment

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

well, I found this quite unusual. But it is probably just my taste so NP.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 85.14% when pulling 4f08cbc on joseivanlopez:guided_setup into d549d5a on yast:master.

Copy link
Member

@jreidinger jreidinger left a comment

Choose a reason for hiding this comment

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

LGTM

@joseivanlopez joseivanlopez merged commit 92c8733 into yast:master Apr 12, 2017
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