-
-
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 clarifying message to several empty panes #20767
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
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 for your work here @jsbautista ! Left some initial suggestions about the new helper widget class definition and the propagation of such a change in the places where it is used currently.
Also, seems like some test suite jobs are still failing, not totally sure why 🤔
Let me know if you have any question!
Great work @jsbautista! Quick feedback from the screenshots you posted: |
Co-authored-by: Daniel Althviz Moré <[email protected]>
Co-authored-by: Daniel Althviz Moré <[email protected]>
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.
Some small suggestions for you @jsbautista and @dalthviz, then this should be ready for me.
But I'd also like to ask @CAM-Gerlach to give us a hand with the texts we're going to show in empty panes.
Co-authored-by: Carlos Cordoba <[email protected]>
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.
Standard reminder: You can directly apply all the suggestions you want in one go by going to Files changed
-> Clicking Add to batch on each suggestion -> When done, clicking Commit
I really like the new designs overall; looks a lot more polished, slick and friendly, as well as VSCode-esque. This will definitely be a nice improvement! Great choices of icons, too.
One key high-level question—are these shown only the first time the pane is empty after Spyder start (which is what I'm assuming, given the current wording), or are they also displayed if the pane later become blank after having content (e.g. closing a project, switching to an unprofiled or un-analyzed file, etc)? If the latter, then the UI text needs some substantial further revisions.
One high level comment: At least in the demo GIF above, the text box jumps around a lot with inconstant height, width and borders between the different panes. Is the GIF outdated, or is this still the case? If so, could that be made more consistent?
Some things across the UI text suggestions:
- I would avoid the "you" here, as it could come across as a bit accusatory and demanding, implying it is the user's fault for not searching for anything. Instead, I rephrase things accordingly to avoid this, which generally ends up being more direct and concise as well as more consistent with how this is presented elsewhere.
- Be consistent about using a period for the description text, but not the summary line
@dalthviz, you need to merge again with master to get the fixes to our tests. |
@dalthviz, all the latest suggestions are ready for you to apply them. But please be sure to read our conversation carefully to pick up the right ones, hehe. |
They are displayed every time the pane becomes blank. For instance, we show the Variable Explorer message on kernel restarts and new consoles. However, we can discuss about that behavior in a follow-up issue (and change it, if we consider it necessary).
That depends on the widgets and layout of each pane. I think there's little we could do to make these messages look exactly the same for all panes. But margins and padding are standard for all of them. |
@dalthviz, I noticed that when searching in the Find pane, results are not shown correctly on the pane. This is what I'm seeing: Please fix that before merging this PR. |
Co-authored-by: C.A.M. Gerlach <[email protected]> Co-authored-by: Carlos Cordoba <[email protected]>
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 @jsbautista and @dalthviz for your hard work on this one! It's a really nice improvement!!
@dalthviz, please squash-merge this one because it has a lot of back and forth around code.
Okay, that's fine, I don't have a particular problem with the behavior and am not proposing to change it. However, the reason I was asking, though, is because it is quite relevant for the UI text you asked me to review and revise, because a lot of the existing text (from before I revised it, as well as the current text) implies or presumes that these messages will only be shown the first time the pane is blank, and has parts that don't make sense otherwise, so I'll need to tweak my previous revisions accordingly as a lot of them were incorrect, based on the original text that carried this assumption.
Okay, not a blocker then 👍 |
Do you prefer to do the new text revision here or in a follow up PR @CAM-Gerlach ? |
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.
I've further tweaked the "title" message for each relevant pane to accurately reflect when it is actually shown, avoid confusion, more clearly inform the user and fix a couple related issues.
Since it is a direct followup to my previous requested UI text review to correct the defects therein, based upon my inaccurate initial presumption when writing that (in turn based on the previous copy), and only involves making small tweaks to a few existing lines of UI text, I've submitted a review with one-click applyable suggestions to fix this issue here. |
I agree with @CAM-Gerlach's suggestions. |
Co-authored-by: C.A.M. Gerlach <[email protected]>
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.
LGTM from me, from a UI text perspective 👍
Description of Changes
Empty panels feedback preview:
Issue(s) Resolved
Fixes spyder-ide/ux-improvements#11
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:
@jsbautista