search

mozilla / FirefoxColor

Theming demo for Firefox Quantum and beyond
https://color.firefox.com
Mozilla Public License 2.0
461 stars 95 forks source link

Fixes #335: Fix autoscroll wrongly activated #897

Closed colibie closed 4 years ago

colibie commented 4 years ago

@Rob--W, I was able to install the project by deleting the node_modules and running npm install again. I tried your suggestion of using e.preventDefault() but it didn't work. I also tried adding return false to the click functions which also didn't work.

I then now replaced href='#' with href='javascript:void(0). This however had the effect of opening a new tab when the middle scroll button is clicked.

Alternatively, I replaced the anchor tags with a span tag so that a new page isn't opened on middle scroll click. That is, nothing happens when the middle scroll button is clicked.

I am using a windows OS and do not know if the resolution has the same effect on Linux and Mac

Rob--W commented 4 years ago

Hy @Je-ni , thanks for the patch. The diff looks completely different, did you inadvertently change the line endings (e.g. as a feature of your text editor)? Could you undo the line changes?

event.preventDefault() should have worked. Have you tried inserting a debugger; statement to see if the code ran at all?

colibie commented 4 years ago

Hy @Je-ni , thanks for the patch. The diff looks completely different, did you inadvertently change the line endings (e.g. as a feature of your text editor)? Could you undo the line changes?

event.preventDefault() should have worked. Have you tried inserting a debugger; statement to see if the code ran at all?

Ohhh, I'm sorry about the line endings. I'll change it. I did not add a debugger. Let me check it too.

colibie commented 4 years ago

@Rob--W please review this.

Rob--W commented 4 years ago

The change itself looks good, but there are some unrelated commits in this PR that needs to be omitted. I guess that you've merged the master branch to keep your local branch up to date. Keeping the branch updated is good practice, but you should rebase instead of merge to avoid unrelated commits in your branch / pull request.

Can you rebase your local branch? If you're using the command line, use the interactive mode (-i / --interactive) to select the commit of interest (and drop the others): git rebase origin/master -i . After rebasing your local branch, you have to force-push to update the pull request (by default, git refuses to rewrite the already-pushed commits, but when you are working on your own branch, overriding this check is okay): git push origin 335-autoscroll-wrongly-activated -f (this force-pushes your current branch to your remote branch, and the update will be reflected in this pull request).

For more information on rebasing, see https://help.github.com/en/github/using-git/about-git-rebase and https://git-scm.com/book/en/v2/Git-Branching-Rebasing

colibie commented 4 years ago

@Rob--W I think it looks cleaner now.

colibie commented 4 years ago

😊😊 my first official PR merge. Thanks @Rob--W