Cleanups #7
Open
Cleanups #7
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This simplifies the code a bit for handling invalid inputs. Doing this also removes the necessity for `ErrorText`
Previously it was an array, though it always had only one element. It might have been introduced to map multiple keys to the same action, but that was never implemented. In my opinion this makes it a little bit easier to understand
This is taken care of the next line `!settings.smoothScroll` which would be true for `undefined` as well
This tried to access `chrome.extension`, which obviously does not exist in Safari. Also it is not used anywhere
Since the function that is called depending on the value is empty this does not have "real life consequences" if it was called multiple times, but if that function should be kept at least we can fix the typo so that future devs do not run into this
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hey!
Great that you picked up the work and even improved it with a nice UI! I had a first glance at a few of the files and made a couple of suggestions for cleanups (and also one commit that fixes a spelling in a JS variable that basically is not needed any more). These are all subjective, so feel free to drop any of the commits as you like (especially I am not sure about the keybindings being string arrays and what the plan was for them)
Could you please give it a try? I am not sure if I broke something with the keybindings - some are working for me (like
f), butj/k/G/g gdon't and I don't know if that is because of my changes (this is the first time I am working on a Safari extension and my debugging skills could still be improved...)