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

Address getGamepads changes #72

Merged
merged 2 commits into from
Jan 11, 2022
Merged

Conversation

Christian7573
Copy link
Contributor

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 ¯_(ツ)_/¯

- Added dashboardWarningMessage to Status state
- Display dashboardWarningMessage in OpModeView
- Prevent app from crashing if navigator.getGamepads is not defined
- Display dashboard warning if navigator.getGamepads is not defined
@rbrott
Copy link
Member

rbrott commented Nov 28, 2021

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
Copy link
Contributor Author

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
Copy link
Member

rbrott commented Dec 7, 2021

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
Copy link
Member

rbrott commented Jan 9, 2022

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?

- Removed dashboardWarningMethod logic
- Added gamepadsSupported to Status state
- gamepad_not_supported.svg is displayed in place of gamepad.svg when
  gamepads are not supported
- (Questionable) Tooltip logic informing the user that the gamepads are not
  supported.
@Christian7573
Copy link
Contributor Author

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
Copy link
Contributor

NoahBres commented Jan 10, 2022

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
Copy link
Member

rbrott commented Jan 11, 2022

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.

@rbrott rbrott merged commit a1786b8 into acmerobotics:master Jan 11, 2022
@NoahBres NoahBres mentioned this pull request Nov 21, 2022
j5155 pushed a commit to jdhs-ftc/ftc-dashboard-record that referenced this pull request Jul 22, 2024
* Address getGamepads changes

- Added dashboardWarningMessage to Status state
- Display dashboardWarningMessage in OpModeView
- Prevent app from crashing if navigator.getGamepads is not defined
- Display dashboard warning if navigator.getGamepads is not defined

* Unsupported Gamepads now conveyed through icons

- Removed dashboardWarningMethod logic
- Added gamepadsSupported to Status state
- gamepad_not_supported.svg is displayed in place of gamepad.svg when
  gamepads are not supported
- (Questionable) Tooltip logic informing the user that the gamepads are not
  supported.

Co-authored-by: Christian7573 <cag4561@gamil.com>
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.

Dashboard breaks on Firefox - navigator.getGamepads is not a function
3 participants