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.

Sign up for GitHub

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

Jump to bottom

Popup.YesNo(): Unify the internal [No] button ID (bsc#1193326) #1208

Merged
merged 2 commits into from
Dec 2, 2021
Merged

Popup.YesNo(): Unify the internal [No] button ID (bsc#1193326) #1208

merged 2 commits into from
Dec 2, 2021

Conversation

lslezak
Copy link
Member

@lslezak lslezak commented Dec 2, 2021

  • Unify the internal button IDs
  • The new Yast2::Popup class use the :yes and :no IDs while the old Popup module uses the :yes and :no_button IDs
  • That's inconsistent and makes testing via libyui REST API more difficult as you need to know which implementation is used to display a particular popup
  • So let's use the same IDs everywhere 👍
  • Additionally the code has been refactored and simplified

@coveralls
Copy link

coveralls commented Dec 2, 2021 •
edited
Loading

Coverage Status

Coverage increased (+0.07%) to 41.148% when pulling e00d55d on no_button into 5ff8848 on master.

jreidinger
jreidinger reviewed Dec 2, 2021
View reviewed changes
library/general/src/modules/Popup.rb
yes_button = PushButton(Id(:yes), Opt(*yes_opts), yes_button_message)
no_button = PushButton(Id(:no), Opt(*no_opts), no_button_message)

ButtonBox(yes_button, no_button)
Copy link
Member

Choose a reason for hiding this comment

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

it a bit scary me to refactor it without test coverage in https://github.com/yast/yast-yast2/blob/master/library/general/test/popup_test.rb but if you test it enough manually then it has green from me

Copy link
Member Author

Choose a reason for hiding this comment

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

Tested manually, works fine.

@lslezak lslezak merged commit f2863b8 into master Dec 2, 2021
@lslezak lslezak deleted the no_button branch December 2, 2021 15:22
@yast-bot
Copy link
Contributor

yast-bot commented Dec 2, 2021

✔️ Public Jenkins job #334 successfully finished
✔️ Created OBS submit request #935230

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jreidinger jreidinger jreidinger approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@lslezak @coveralls @yast-bot @jreidinger