-
Notifications
You must be signed in to change notification settings - Fork 178
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
Draft: Some usability improvements, and grammar fixes. #327
base: main
Are you sure you want to change the base?
Conversation
- The code that automatically pads the bottom of the page so that the user is still able to scroll down and see the bottom of the page even with the console expanded now works on all pages (previously was only working correctly on error pages). - Improve keyboard shortcut handling for 'Home', 'End', 'Ctrl-P', 'Ctrl-N', and 'Ctrl-E'. - 'Home' and 'End' go to beginning and end of line; 'Ctrl-E' end of line; 'Ctrl-P' and 'Ctrl-N' navigate forward and back through command history, just like up and down arrows, to match default key mappings in bash. - Add .moveToBeginning and .moveToEnd helper functions to the REPLConsole JS, and use them in key input handlers to make them easier to read. - Improve styling: font sizes and heights are in rems to adjust to font size in browser; close button is a little bigger and offset from the right margin to be more easily usable in browsers that render the scrollbar over the content. (Some widgets in Gnome shell in Ubuntu do this, maybe others also.) - Some small grammar and English usage fixes in comments and README. Details: - Add a container element around the web console so we can more easily add padding to keep it from obscuring the page - DRY the code that adds a margin to the bottom of the page so that user is still able to scroll down and see the bottom of the page even with the console expanded; make it so it works whenever the console is loaded, not just on error pages. - Remove the extra now-redundant code from the error_page.js.erb and regular_page.js.erb; it now lives in main.js.erb. - It wasn't actually working before on regular pages anyway. - Change font sizes used in console from pixels to rems so they will auto-adjust to font size used on the page. This also makes them just a little bigger by default (unless the page font size is very small). - Increase the size of the "close" button slightly so it is easier to hit with the mouse, and also move it slightly away from the right side, so that it doesn't render under the scrollbar on some browsers (usually on Linux, in my experience; the default Gnome shell scrollbars in Ubuntu 20 made it very hard to use). - Add key handling for 'Home' and 'End' keys. - Change Ctrl-E handler to move cursor to end of line, to match default bash builtin key mappings. - Add Ctrl-P and Ctrl-N handlers to navigate previous and next in command history. - Add .moveToBeginning and .moveToEnd helper functions to the REPLConsole JS, and use them in key input handlers to make them easier to read. - Fix grammar and English usage just a little bit in some comments and README.
It would be more ideal if I added some new tests to cover my changes, but the existing tests still pass, at least. |
Can we separate 63487ff into two commits – one for the grammar fixes and one for the UX improvements? |
@@ -1,5 +1,5 @@ | |||
# frozen_string_literal: true | |||
|
|||
module WebConsole | |||
VERSION = "4.2.0" | |||
VERSION = "4.2.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.
Let's bump the version to 4.3.0
.
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.
Ok, thanks.
I'll test it locally and give you more feedback later on. Thanks for the effort so far! |
Yeah, will do that; thanks ... |
Ok, sorry I dropped the ball on this. I still want to merge it if possible, and I have made further changes on top of these in my fork to allow persistent minimize/restore of the console, which I want to put in a second PR. How do you recommend I split the commits? Should I reset / redo the commit, and then force push, or should I add a commit to revert the original commit, and then add 2 new commits? Does it matter which way I do it? Thank you very much. |
Summary:
Details:
Add a container element around the web console so we can more easily add padding to keep it from obscuring the page
DRY the code that adds a margin to the bottom of the page so that user is still able to scroll down and see the bottom of the page even with the console expanded; make it so it works whenever the console is loaded, not just on error pages.
Change font sizes used in console from pixels to rems so they will auto-adjust to font size used on the page. This also makes them just a little bigger by default (unless the page font size is very small).
Increase the size of the "close" button slightly so it is easier to hit with the mouse, and also move it slightly away from the right side, so that it doesn't render under the scrollbar on some browsers (usually on Linux, in my experience; the default Gnome shell scrollbars in Ubuntu 20 made it very hard to use).
Add key handling for 'Home' and 'End' keys.
Change Ctrl-E handler to move cursor to end of line, to match default bash builtin key mappings.
Add Ctrl-P and Ctrl-N handlers to navigate previous and next in command history.
Add .moveToBeginning and .moveToEnd helper functions to the REPLConsole JS, and use them in key input handlers to make them easier to read.
Fix grammar and English usage just a little bit in some comments and README.