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 scroll capacities to menu #122

Merged
merged 34 commits into from
Apr 8, 2020
Merged

Add scroll capacities to menu #122

merged 34 commits into from
Apr 8, 2020

Conversation

anxuae
Copy link
Contributor

@anxuae anxuae commented Apr 7, 2020

This is the full implementation of #15.

@codecov
Copy link

codecov bot commented Apr 7, 2020

Codecov Report

Merging #122 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #122   +/-   ##
=======================================
  Coverage   84.30%   84.30%           
=======================================
  Files          27       27           
  Lines        3314     3314           
=======================================
  Hits         2794     2794           
  Misses        520      520           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 127723e...127723e. Read the comment docs.

@lgtm-com
Copy link

lgtm-com bot commented Apr 7, 2020

This pull request introduces 6 alerts when merging 857d36b into 0387712 - view on LGTM.com

new alerts:

  • 4 for Wrong number of arguments in a call
  • 1 for Unused local variable
  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Apr 7, 2020

This pull request introduces 6 alerts when merging 37e80f0 into 0387712 - view on LGTM.com

new alerts:

  • 4 for Wrong number of arguments in a call
  • 1 for Unused local variable
  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Apr 7, 2020

This pull request introduces 4 alerts when merging 250665b into 0387712 - view on LGTM.com

new alerts:

  • 4 for Wrong number of arguments in a call

@lgtm-com
Copy link

lgtm-com bot commented Apr 7, 2020

This pull request introduces 4 alerts when merging f5ca803 into 0387712 - view on LGTM.com

new alerts:

  • 4 for Wrong number of arguments in a call

@ppizarror
Copy link
Owner

@lgtm-com
Copy link

lgtm-com bot commented Apr 7, 2020

This pull request introduces 4 alerts when merging 00485d7 into 0387712 - view on LGTM.com

new alerts:

  • 4 for Wrong number of arguments in a call

@lgtm-com
Copy link

lgtm-com bot commented Apr 7, 2020

This pull request introduces 4 alerts when merging 12f5c17 into 0387712 - view on LGTM.com

new alerts:

  • 4 for Wrong number of arguments in a call

@lgtm-com
Copy link

lgtm-com bot commented Apr 7, 2020

This pull request introduces 1 alert and fixes 4 when merging 9afd200 into 0387712 - view on LGTM.com

new alerts:

  • 1 for Wrong number of arguments in a call

fixed alerts:

  • 4 for Wrong number of arguments in a call

@lgtm-com
Copy link

lgtm-com bot commented Apr 7, 2020

This pull request introduces 1 alert and fixes 4 when merging c50562a into 0387712 - view on LGTM.com

new alerts:

  • 1 for Wrong number of arguments in a call

fixed alerts:

  • 4 for Wrong number of arguments in a call

@ppizarror
Copy link
Owner

Added fully column support. Also Added an option to center vertically the menus.

@@ -297,6 +297,20 @@ def get_view_rect(self):

return rect

def get_actual_scrollbar_thickness(self, orientation):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

naming get_scrollbar_thickness is enough , no?

Copy link
Owner

Choose a reason for hiding this comment

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

that's better

@ppizarror
Copy link
Owner

ppizarror commented Apr 7, 2020

@anxuae. There's an inconsistency in draw_region_y.

After the y scrollbar returns to the first position. In the test I made (see gif) at first the margin is valid (added draw_region_y=30), But after returning to the first position the margin was deleted.

https://i.imgur.com/KXUOohR.gif

PD: Not deleted, but it should return to the first state drawn

@lgtm-com
Copy link

lgtm-com bot commented Apr 7, 2020

This pull request introduces 1 alert and fixes 4 when merging b9f9fe7 into 0387712 - view on LGTM.com

new alerts:

  • 1 for Wrong number of arguments in a call

fixed alerts:

  • 4 for Wrong number of arguments in a call

:return: Thickness in px
:rtype: int
"""
if orientation == _locals.ORIENTATION_HORIZONTAL and self._world.get_width() > self._rect.width or \
Copy link
Contributor Author

@anxuae anxuae Apr 7, 2020

Choose a reason for hiding this comment

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

The condition self._world.get_width() > self._rect.widthis not enough (there are some specific cases when world size is close to area size)

Use get_hidden_width() == 0 or get_hidden_height() == 0 to know if scrollbars are displayed or not

Copy link
Owner

Choose a reason for hiding this comment

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

changed!

@anxuae
Copy link
Contributor Author

anxuae commented Apr 7, 2020

@anxuae. There's an inconsistency in draw_region_y.

After the y scrollbar returns to the first position. In the test I made (see gif) at first the margin is valid (added draw_region_y=30), But after returning to the first position the margin was deleted.

https://i.imgur.com/KXUOohR.gif

PD: Not deleted, but it should return to the first state drawn

Fixed ;-)

@lgtm-com
Copy link

lgtm-com bot commented Apr 7, 2020

This pull request introduces 1 alert and fixes 4 when merging a6a66c6 into 0387712 - view on LGTM.com

new alerts:

  • 1 for Wrong number of arguments in a call

fixed alerts:

  • 4 for Wrong number of arguments in a call

@lgtm-com
Copy link

lgtm-com bot commented Apr 7, 2020

This pull request introduces 1 alert and fixes 4 when merging b02de13 into 0387712 - view on LGTM.com

new alerts:

  • 1 for Wrong number of arguments in a call

fixed alerts:

  • 4 for Wrong number of arguments in a call

@ppizarror
Copy link
Owner

As menu has more parameters, I renamed color_selected to selection_color and rect_width to seletion_border_width. This breaks compatibility with previous versions.

@ppizarror
Copy link
Owner

ppizarror commented Apr 8, 2020

Also renamed title_offsetx and title_offsety to title_offset_x and title_offset_y.
Introduced new parameters selection_inflate_margin_x and selection_inflate_margin_y.

@ppizarror
Copy link
Owner

Introduced also a migration guide: https://github.com/ppizarror/pygame-menu/wiki/Migration-Guide

@anxuae
Copy link
Contributor Author

anxuae commented Apr 8, 2020

Hi @ppizarror , just a question, how do you add your own commit to my PR?
Can you explain me what git commands do you use?
Thanks

@ppizarror
Copy link
Owner

Hi @ppizarror , just a question, how do you add your own commit to my PR?
Can you explain me what git commands do you use?
Thanks

Just use github for windows and vscode. Maybe b/c I have the permissions of Owner?

@ppizarror
Copy link
Owner

I think this PR is way too large. I'm gonna merge it, if you want to change anything please create another PR. Thanks!!

@ppizarror ppizarror merged commit fcf0d6e into ppizarror:master Apr 8, 2020
@anxuae anxuae deleted the scrollmenu branch April 10, 2020 12:23
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

2 participants