-
Notifications
You must be signed in to change notification settings - Fork 45
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
Add Shortcuts #958
Add Shortcuts #958
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
- Added overview page for shortcuts - added shortcuts - added translations for shortcuts
8328631
to
8a69cfe
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.
Sorry for the long delay in reviewing this! I also only managed to do a quick review and test. But better than nothing!
I left a few inline comments, but I have additional comments from testing it. In random order:
-
Shortcut overview page
- The shortcut overview is quite "large" and somewhat breaks on screens widths < 480px. Maybe reduce the space between entries and also reduce the padding in the div that displays the key?
- Not sure
<th>
is the right choice for these headings. - Not sure
<table>
is the right choice there, given that you usedisplay: flex
anyway. I think screenreader and stuff doesn't treat it as a table anymore then, so you can also just use a<div>
? - Some entries have three columns in total. Not sure I would do it like that. In particular, it would be nice if for example the "shift del" part of "Delete left crop marker" could could break into a new line on small screens, which is not currently possible.
-
Choice of shortcuts
- Alternative: use 1, 2, 3 for the choices (video and audio source). Then there are fewer different shortcuts.
- Maybe show the shortcut on the buttons when pressing "alt"? Like Windows does in many programs.
- ctrl+enter is already used by browser to open a selected link in a new tab, so it doesnt work in chrome. Neither does ctrl+backspace.
- Cut: maybe pos1 and end?
- Left/right arrow keys for next/back buttons
-
After selecting camera, I'm not sure how to proceed with shortcuts alone (without tab).
-
Menu at top is very wide now, already breaking for screen sizes <660px, which is problematic. I don't have a good fix for this except maybe putting the links to shortcuts (and info?) into settings? But I guess the proper solution is redesign of that section? Unfortunately, the design proposed by Lisa does not really address that. Except for removing the "recording" entry. Maybe one could remove the label and only show the icon for small screen sizes? But then we certainly want toolstips with the label instead.
- indentations - added/removed spaces and braces
This pull request has conflicts ☹ |
- Overview-page is no longer a table - Space between entries is reduced - so the overview is "smaller" and fits better - splitted the keyMap into seperate groups
- added 'defaultKeyEvent: keydown' to config and removed it from the keyMaps - added 'ignoreKeymapAndHandlerChangesByDefault: false' to config so 'allowChanges={true}' does not have to be passed in the <GlobalHotKeys> component each time
- fpsInfo is not stored in localstorage anymore - removed "getElementsByClassName" and replaced it with querySelector and adjusted the loop used by this
- not needed anymore due to last commit
- new/changed shortcuts should now be easier to memorize, there are fewer different shortcuts - now 1, 2 and 3 for choices (when selecting video and audio source) - changed shortcuts for cutting (setting a crop mark) and deleting that crop mark - shortcuts for next and back buttons changed, because they were used by browsers
- Due to new header entry "shortcuts" the header is very large. Now it shows only icons on small screens (screen width < 750px) and Icons with text on large screens
- Resolved conflicts
This comment was marked as resolved.
This comment was marked as resolved.
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.
Generally good changes, but I now reviewed in a bit more detail and found quite a few things that should still be fixed. For some style things (like missing semicolon or wrong spacing), I haven't marked every occurrence, so it would be great if you could go over the changes again and add missing semicolons for example. Same goes for the getElementById
(see relevant comment below). It would also be great if commit message of new commits could follow the rules we recently talked about internal (i.e. this blog post).
Sorry if many of these comments seem very nitpicky, but I do think we should stick to the code style already present. Though I agree Studio could use a better lint-config and/or formatter so that following the style is easier (and CI can flag violations automatically).
I have tested briefly and everything seems to work! I do need a bit more testing before merging this, but that can wait for the next round of reviews.
src/ui/studio/review/index.js
Outdated
var deleteLeft = document.getElementById("deleteLeft"); | ||
var deleteRight = document.getElementById("deleteRight"); |
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.
Getting references to elements in the DOM this way is very unidiomatic in React. This should be solved by a ref
. Giving elements some IDs and then retrieving them that way is very error prone and breaks as soon as some IDs conflict or when one of those strings is not updated.
This applies to several other places as well. In some it's easier to fix as the relevant component/DOM element is rendered by the same component (like here). In other places it's a bit harder and would require passing around refs. At least in the former cases, this should be improved.
src/ui/studio/review/index.js
Outdated
if (deleteLeft !== null){ | ||
deleteLeft.click(); | ||
} |
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.
if (deleteLeft !== null){ | |
deleteLeft.click(); | |
} | |
deleteLeft?.click(); |
Same below
At a screen width of 742px only the icons are now displayed in the header menu and the text is hidden. The 742px are based on the French language (the header is the widest in French) and at a screen width of 742px the last header entry ("Info") disappears. A group of shortcuts no longer has a fixed width. EXCEPT from a screen width of 350px. At about this width, it starts to look too tight and squashed. Therefore the groups are then 300px wide. So it still looks nice with a width of 320px (300px for shortcuts and 10px margin per side).
Implemented as suggested. Couldn't figure out how to replace getElementById("deleteLeft") and getElementById("deleteRight") with references. Will keep trying, it's the last code related suggestion (besides code style/formatting suggestions).
Hopefully addressed all issues related to formatting like missing semicolons, spaces and indentations. Looked up all classes with changes from this PR.
…inding When skipping 5 seconds or a frame (in the review view) and the current time goes beyond the video duration or is below 0 seconds, the video reference is undefined. Solved it by simply checking if the reference is undefined.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Will resolve eslint-errors in next commit
With a screen width of 575px and less, the "menu" button appears, but the text in its dropdown list is hidden. This is caused from the media-query implementet earlier ([see this commit](elan-ev@6b69f55#diff-1530ee2c8b03d767ee576f99d6e76f63ef0ac5d2884bbc11e79780fd5cc3ee59)) I added a second media query that shows the text again, when the dropdown menu appears (Not sure if this is the right way to do this)
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.
My comments were all addressed as far as I can tell. So I think it's finally time to merge this :D
I'm not quite sure how intuitive the implemented shortcuts for Studio are.
Please let me know if you know better/intuitive shortcuts or if you miss some.