-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
PR: Add new combobox widgets (API) #21555
Conversation
@dalthviz, this is ready for review. Could you also check how things look on Windows and Mac? I wouldn't expect big surprises there, but you never know. |
29e56d9
to
5f38924
Compare
Hello @ccordoba12! 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 2023-12-04 18:06:11 UTC |
69b73bc
to
b0c1d78
Compare
SpyderComboBox
widgetSpyderComboBox
widget (API)
Did an initial check on Windows and seems like things are looking good except for the following (although not totally sure if is caused due to this PR or something that was happening before):
On macOS there is a more or less noticeable misalignment for the comboboxes- The Find in Files |
7454f25
to
89ee9bd
Compare
SpyderComboBox
widget (API)492e043
to
7218573
Compare
Thanks @dalthviz for the review! About your comments:
Fixed.
I plan to fix that after this PR is merged. Actually, my initial motivation for this work was to fix that problem.
Fixed. I also made the two QFontCombox'es we are using in Preferences to follow the style of the other comboboxes. See the updated OP for the screenshots. |
Checked again with the latest changes on Windows and now seems like some comboboxes that have an initial value appears with a white background? This is what I'm seeing for the Help pane And it also happens with the Find pane Also, checked the working directory combobox but it still shows the same behavior 🤔 Also, regarding the comboboxes to select fonts seems like now the items doesn't show a preview of the font? Is not that kind of a regression or maybe for that kind of combobox a new preview widget is meant to be done? |
I was able to reproduce this and it should be fixed now.
Really fixed now. I was not seeing this problem on Linux, so I thought some of my previous changes should have fixed it.
I understand your point of view but I was not able to find a way to set a specific font for combobox items. And I think the current style doesn't look good, as can be seen in the OP. Furthermore, I also checked what RStudio does and it simply shows the font names, as we are doing now. |
Checked again and seems like things are working now 👍 Regarding the fonts comboboxes, I would suggest to make the Following that idea I would also suggest to create a second preview widget for the |
This makes clearer how to customize its style.
- This has a style slightly different from SpyderComboBox. - Use it in the ClassFunctionDropdown panel.
Also, fix top/bottom padding because it was too big.
That's because we have two comboboxes now in that module.
This is a subclass of QFontComboBox that follows the style of our other comboboxes.
a99dc9a
to
38e4b13
Compare
Great!
Great suggestion @dalthviz! I haven't thought about doing something like that, but I agree it'd help users to understand how font changes would affect the interface. So, I implemented that in my last two commits.
That's a good suggestion too, but I think out of the scope for this PR. I have some ideas on how to implement it but for that I'd require PR #21511 merged first. So, I think it's better to leave it for later. |
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 @ccordoba12 ! Just left a comment regarding completly removing the context menu from comboboxes other than that this LGTM 👍
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 @ccordoba12 !
Just in case, is there anything else you want to add here @ccordoba12 ? I just noticed that this is marked for beta1 🤔 |
Nop. I thought it was going to take longer to get this PR approved, so I didn't want to delay alpha3 due to that. But since you think it's ready, let's include it in that release. |
Description of Changes
This allows us to improve the style of all comboboxes used in the application.
UI improvements:
UX improvements:
Other changes:
Visual changes
Before
After
Affirmation
By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.
I certify the above statement is true and correct: @ccordoba12