search

acmerobotics / ftc-dashboard

React-based web dashboard designed for FTC
https://acmerobotics.github.io/ftc-dashboard
Other
178 stars 137 forks source link

Address getGamepads changes #72

Closed Christian7573 closed 2 years ago

Christian7573 commented 2 years ago

Handle navigator.getGamepads() not being defined instead of crashing, and display a warning to the user. Fixes #67. I've tested in Chrome and Firefox.

There is a setTimeout in gamepadMiddleware.ts that I'm not proud of, but it told me I couldn't dispatch while constructing the middleware. I've never touched React before, so I wouldn't know if there's a better way of achieving that ¯_(ツ)_/¯

rbrott commented 2 years ago

Thanks for the patch! Well done, especially considering your lack of React experience. The whole middleware mania was a bit misguided anyway.

I like the explicitness of the sticky warning, but I worry that its persistence will annoy users and distract from normal RC errors.. I gather (perhaps incorrectly) that there are few gamepad users, and I'm more partial to a subtle indication like slashed gamepad icons with an explanatory tooltip.

As always, I'm open to hearing alternate proposals.

Christian7573 commented 2 years ago

I'll see if I can manage crossing out the gamepads, CSS strikethrough property will probably cover it. When I do so would you like me to keep or discard the dashboardWarning related code.

rbrott commented 2 years ago

I'll see if I can manage crossing out the gamepads, CSS strikethrough property will probably cover it.

Strikethrough may work, though I'd recommend swapping out the normal icon for this one. Let me know if you want further pointers on the icon plumbing.

When I do so would you like me to keep or discard the dashboardWarning related code.

I'd prefer to keep that code out until a future juncture when a better warning candidate appears.

rbrott commented 2 years ago

I'd love to land this change or something similar in the next few weeks. Were you planning on making progress in that time frame?

Christian7573 commented 2 years ago

Successfully implemented. Sorry for the delay!

I had some trouble getting your useDelayedToolTip function to work, it kept throwing errors relating to useRef and useState. Google lead me to believe the errors it threw were because OpModeView is a class-based component rather than a function-based component. I implemented some hover logic in the fields & state of OpModeView instead.

NoahBres commented 2 years ago

For future reference, in case you're learning react, useDelayedToolTip is a hook and hooks are designed to be used in functional components. The functional based components w/hooks have a very different paradigm (no defined lifecycle, instead relying on algebraic side-effects, etc.) compared to class based components which are being phased out of use.

However, both types of components can exist in a tree so the solution to using them would be to create a separate functional component and just stick it in your class render function.

Not necessary ofc! Just a hook for managing the tooltip state.

rbrott commented 2 years ago

That was quick! I agree with Noah that the component should be made functional and use the hook at some point in the future. I'm otherwise pretty happy with the implementation.