-
-
Notifications
You must be signed in to change notification settings - Fork 283
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
Overhaul Editor doc with many new sections and screenshots #331
Overhaul Editor doc with many new sections and screenshots #331
Conversation
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 this amazing work @hyounes4560 ! I left some comments, mainly there are some issues with the rendering and consistency among the way shortcuts are displayed.
Co-authored-by: Stephannie Jimenez Gacha <[email protected]>
Thank you; this worked.
@CAM-Gerlach I don't have anything else to push; the only missing thing here is some GIFs. I've shared the videos with you and as per a previous discussion, you could convert them to GIFs. Please let me know if you haven't received them Thanks again |
Great!
Oh, sorry about that! I had just gotten back from the Python Core Developer Sprint and was heavily backlogged in the aftermath of that, and got myself mixed up somehow thinking that this PR was pending the remaining screenshots/GIFs, rather than waiting on me to process them. My bad! In any case, I have and will be very busy for the next week preparing for the big Python 3.11 release next Monday (I'm in charge of the What's New in Python 3.11 and also making sure everything is documented for the release, so I can't promise I will be able to find the spare time until then, but I'll do my best. Otherwise, I'll take care of it perhaps around the middle of next week, if that's okay. Thanks again for all your hard work, and looking forward to seeing this land soon! |
@CAM-Gerlach no worries; take your time. |
@ccordoba12 and @dalthviz can you please do the final review to see if we can merge this PR by next week? Thanks!! |
487200c
to
c3dda6e
Compare
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 again for your hard work on this @hyounes4560 ; the GIFs look pretty good—I just had to reprocess them to fix their size and other issues (see important note below). With that, LGTM now from me!
|
Thanks @CAM-Gerlach, we will just wait for the review of the rest of the team to merge this PR! |
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 all the work here @hyounes4560 @steff456 and @CAM-Gerlach ! It's a huge improvement! Left a simple comment regarding a text role that maybe needs a change on the Run file section but other than that this LGTM 👍
I will re-generate the GIFs using the script above and latest original source videos as soon as I get confirmation of those, to improve the clarity and quality. In the meantime, @steff456 would you like to formally approve this as well? 🙂 |
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 @hyounes4560 for your impressive work on this PR! I left some small suggestions, but overall looks pretty good to me.
Also, thanks to @steff456 and @CAM-Gerlach for your help during this process.
Co-authored-by: Carlos Cordoba <[email protected]>
Since the changes @ccordoba12 requested were all quite small and straightforward, aside from one that was a theme change, I went ahead and took care of applying them to expedite final merging of this PR. As discussed previously, I regenerated the GIFs from their source videos; while the quality improvements due to that were rather minimal, as apparently there was still a significant quality issue with the original videos (which seemed likely due to scaling/rescaling, either on capture or during its processing), it did substantially reduce the final file size even further, and I also increased the output resolution and tweaked the scaling factors to extract as much quality from the originals as practicable, which resulted in modest clarity improvements and a further 28%/1.4x reduction in total file size, for a total savings decrease of 24x vs. the original. Parameters are below, for reference: Conversion parametersFILENAME="editor-automatic-formatting"; CROP=; SCALE="720:-1"; START=; END=; SPEED=0.5; PAUSE=2;
FILENAME="editor-class-function-selector"; CROP="736:ih-2:0:2"; SCALE=; START=01; END=; SPEED=0.5; PAUSE=2;
FILENAME="editor-file-switcher"; CROP=; SCALE="720:-1"; START=; END=; SPEED=0.75; PAUSE=2;
FILENAME="editor-go-to-definition"; CROP="iw:ih-20:0:4"; SCALE=; START=; END=; SPEED=0.75; PAUSE=2;
FILENAME="editor-go-to-line"; CROP="iw:ih-32:0:32"; SCALE="720:-1"; START=; END=; SPEED=0.75; PAUSE=2;
FILENAME="editor-split-panels"; CROP="iw:ih-40:0:40"; SCALE="720:-1"; START=; END=; SPEED=0.5; PAUSE=1;
FILENAME="editor-syntax-highlighting"; CROP="1578-320:910-226:320:226"; SCALE="720:-1"; START=02; END=07; SPEED=1; PAUSE=1;
FILENAME="editor-tabs-sorting"; CROP="1440:880-46:0:46"; SCALE="720:-1"; START=; END=; SPEED=0.75; PAUSE=2; Finally, I spotted and fixed a few final trivial textual polish issues, so this should be now all finally ready to merge. |
Since myself and @dalthviz have approved, and @steff456 and @ccordoba12 's final suggestions were all applied and they both indicated informally that they approved merging this, I'll go ahead with the merge now, so this doesn't have to wait any longer to go live and start benefiting Spyder's users, who I'm sure will be very happy to see the fruits of your tireless efforts here. Thanks so, so much to @hyounes4560 for all her amazing hard work on this PR and her patience, kindness and understanding in the process, even when it wasn't nearly as smooth as it could have been due to my mistakes and being very busy on other things. Thanks to @steff456 for managing this project, guiding and mentoring Hanan, and contributing a lot to the final product. And finally, thanks to @dalthviz and @ccordoba12 for reviewing this and providing great feedback. Cheers! |
Pull Request
Pull Request Checklist
Description of Changes
This is still WIP/draft as I am still working on the rest of the images/GIFs. I used the Image/Image Placeholder test instead of the actual images for now.
Issue(s) Resolved
Fixes #97