Skip to content
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

fix: mouse wheel and arrow key functions in dash #1574

Merged
merged 1 commit into from
Nov 5, 2019

Conversation

bolivierjr
Copy link
Contributor

I have the mouse wheel and arrow keys functioning pretty good now in the dashboard. Let me know what you think and if it needs anymore detail for now?

issue #1572

@vercel
Copy link

vercel bot commented Nov 4, 2019

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

@DreadKnight DreadKnight temporarily deployed to ancientbeast-1572-mouse-bwlojv November 4, 2019 03:32 Inactive
@bolivierjr
Copy link
Contributor Author

Woah, what happened there? I'm guessing the pre-commit and linter/formatter took over?

@DreadKnight
Copy link
Member

@bolivierjr It sure seems so! @ktiedt changed some linter rules recently with a patch while fixing something, in order to have a style more similar to TypeScript afaik xD

@bolivierjr
Copy link
Contributor Author

@DreadKnight Ok, thought I had broke something there for a second, haha! Well, that makes it a litte bit harder to see what I have changed. My bad! xD

@DreadKnight
Copy link
Member

@bolivierjr No worries, seems to be working fine! Will do a bit more testing before merging though, just to be safe.

@bolivierjr
Copy link
Contributor Author

bolivierjr commented Nov 4, 2019

@DreadKnight I did see that bug with the materialization bar disappearing on the blue side when moving around the grid. I'm pretty sure that was doing that before I started messing around with anything. I'll have to check. I see it's coming from here when it hides.
Btw, I see I left the old parameter above in the docstring/comments I took out of the showCreature function and did not edit it to match. Want me to make another commit?

@DreadKnight
Copy link
Member

@bolivierjr I deleted that comment of mine right after because for some reason I couldn't reproduce that bug anymore, so thought it was either a cache problem or something related to recent commits regarding improving behavior with the materialization button. Feel free to improve things anytime!

@ktiedt
Copy link
Collaborator

ktiedt commented Nov 4, 2019

@bolivierjr It sure seems so! @ktiedt changed some linter rules recently with a patch while fixing something, in order to have a style more similar to TypeScript afaik xD

Yeah, a few things were never set right and some rules will reduce diff noise once all the files are in sync again. I suppose it would make a lot of sense to do this in a second ticket to avoid noise on small tickets like this.

@DreadKnight
Copy link
Member

DreadKnight commented Nov 4, 2019

@ktiedt yeah, was thinking about that as well; it's the best solution, as I'm leaning towards TS anyway.

@bolivierjr
Copy link
Contributor Author

bolivierjr commented Nov 4, 2019

looks like the value for "array-element-newline" should be ["error", "consistent"] or ["warn", "consistent"] in .eslintrc.json for the linter to work.

@ktiedt
Copy link
Collaborator

ktiedt commented Nov 4, 2019

@bolivierjr are you seeing an error from it? In the tests I did, the setting (which came from examples) worked.. the explanation being:

"Consistent" should never produce a warning or an error because the array will always be shifted to (my understanding) the most consistent version of it.

[1, 2, 3] // nothing done
// nothing done here either.
[
1,
2,
3
]
[ 1, 2, 3,
4] // I believe this would rewrite to be single line

[1,2,
3,
4,
5
] // I believe this will split with new lines.

@bolivierjr
Copy link
Contributor Author

bolivierjr commented Nov 4, 2019

@ktiedt I only get errors from prettier with array-element-newline setup like consistent, here. I do get errors from array-element-newline if I use the "always" or "never" settings.

Here is what is showing up for me with it set to consistent: https://pastebin.com/KpW6BYQR

@ktiedt
Copy link
Collaborator

ktiedt commented Nov 4, 2019

Have you updated your project packages? We were severely behind on those which will error because those options didn't exist in the old versions.

@bolivierjr
Copy link
Contributor Author

bolivierjr commented Nov 4, 2019

hmm, I'm using node v8.16.2 installed from nvm. Just like the CI build is using and doing npm ci to install packages. No updates or changed files.

@bolivierjr
Copy link
Contributor Author

bolivierjr commented Nov 4, 2019

ohhh, do you mean an actual error from npm at the end of the lint? Because it is showing that at the end of the pastebin. if I run lint-fix, no errors at all though, just one warning. Sorry, thought you were talking about the linter itself giving out errors from bad format.

@bolivierjr
Copy link
Contributor Author

bolivierjr commented Nov 4, 2019

@ktiedt Ah, to my understanding that eslint command without the --fix flag, is supposed to throw error and exit status out of there with a failure if anything is out of format. If using it with CI, it will block any PR or merge coming in until the developer fixes the issues. If you want it to automatically try to fix and pass, then that's where the --fix flag comes in. e.g. I can get npm run lint to pass if i run lint-fix first and it fixes everything correctly.

@ktiedt
Copy link
Collaborator

ktiedt commented Nov 4, 2019

Wow, you should at least be on Node 10 (LTS) its possible you are behind on package versions due to out dated Node binary... Node 12 was just made the new active LTS last month, currently it does not appear to break anything with AB either.... nvm install --lts ;)

hmm, I'm using node v8.16.2 installed from nvm. Just like the CI build is using and doing npm ci to install packages. No updates or changed files.

@bolivierjr
Copy link
Contributor Author

bolivierjr commented Nov 4, 2019

Oh, I see that the package.json says node engine 10.x now. My bad! Though, your TravisCI build log is building with node 8. Might want to update that as well. That's what I was testing the linter with since that's what I noticed in the build log. I have nvm installed with versions 8-12 for different projects .

@ktiedt
Copy link
Collaborator

ktiedt commented Nov 4, 2019

I wasnt involved in the Travis setup, but you are right, we should address that too! Heads up @DreadKnight ;)

@bolivierjr
Copy link
Contributor Author

bolivierjr commented Nov 4, 2019

hm, even with it just set to "consistent" when using node v10 or v12 with packages re-installed. I still get the same error unless I turn it back to ["error", "consistent"]. It only works that way or with a plain 0-2 integer for value. What docs were you looking at when you saw "consistent" only as value? I'm failing to find that.

@DreadKnight
Copy link
Member

I've updated the travis CI node to lts, still fails because we need to update most files according to rules I guess.
Anyway, regarding the PR itself, the materialization button does vanish after a big of play time it seems.

@DreadKnight
Copy link
Member

Did more testing, it seems to be happening before. So I'll merge this and report new issues individually.

@DreadKnight DreadKnight merged commit 69ddb4c into FreezingMoon:master Nov 5, 2019
@bolivierjr bolivierjr deleted the 1572-mousewheel-fix branch November 8, 2019 05:40
CyberBishop pushed a commit to CyberBishop/AncientBeast that referenced this pull request Apr 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants