-
Notifications
You must be signed in to change notification settings - Fork 163
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
Adding SpinBoxUI
to the UI
module
#499
Conversation
Hello @ganimtron-10! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2022-08-19 16:40:36 UTC |
Codecov Report
@@ Coverage Diff @@
## master #499 +/- ##
==========================================
+ Coverage 84.32% 84.49% +0.16%
==========================================
Files 44 44
Lines 10370 10454 +84
Branches 1407 1411 +4
==========================================
+ Hits 8745 8833 +88
+ Misses 1255 1252 -3
+ Partials 370 369 -1
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ganimtron-10,
Thank you for doing this! Overall, it looks good to me. I need to play a bit with these new UI elements because I am not sure that is draggable. if not, I recommend you add an option for this.
The logic for set_value
is strange to me. If I do set_value(10)
I would expect my value to be 10 and not in/decremented by 10. value
should be a property where you can set whatever you want. increment(self)
and decrement(self)
function should be added and used in your de/increment_callback
Hi @skoudoro !
For this, At first I too used the same approach of creating two different functions but both the functions were replication of each other just differing at in/decrement so I created a single function, but I should have given it more descriptive name! I will update the code according to the comments! ASAP. |
I have updated the callbacks and made |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ganimtron-10,
See below for some comments to improve this UI. it is close to being merged.
- When you change the SpinBox size, there are many alignment issues.
- When I
min_val=10
and we reach it, the cone is still rotating. is it normal? same withmax_val
TextBox2D
is editable. when we edit it on the scene, it would be good to catch this event and rotate the cone.
Thanks for the future update!
Hi @skoudoro , |
Hey @skoudoro , |
Hey @skoudoro , |
Hello @skoudoro, |
I have updated this branch! |
Thank you @ganimtron-10. I will be able to look at #790 later this week. From next week, the review rhythm will accelerate and be faster |
What is the update on this PR @ganimtron-10 ? Since, |
This would be ready once we merge #830, it helps to easily implement the modification done to the |
Hey @skoudoro, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ganimtron-10,
See below my comments. it needs some update. Please, let me know when it is done
Hey @skoudoro , I have updated the validating part for the textbox input and the tests completed successfully. Regarding the text touching the borders, I checked the positions for both the text actor and the background, and they are at the same position.
I checked other elements too and this issue is not specific to |
Thank you for the update. I will look into it in a 1h
Ok, I see, can you start a PR to fix that. Let's try to fix this issue this week. we can do a pair coding session on Friday to finalize what you will start today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ganimtron-10,
All good! Just missing one thing:
can you add your example inside the ui section of _valid_examples.toml
Then we can merge the PR and create a new PR concerning issue with the TextBlock2D and TextBox2D
Hey @skoudoro , |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM! merging
Now, let's look at the issue with the TextBlock2D and TextBox2D ! |
Creating a SpinBoxUI with tutorials and tests.