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

Kiosk: Support event-based navigation #9700

Merged
merged 8 commits into from
Oct 5, 2023

Conversation

eanders-ms
Copy link
Contributor

@eanders-ms eanders-ms commented Sep 28, 2023

This PR is huge, I apologize. It contains two main changesets: the switch to event-based navigation and some aesthetic changes to align more closely to Claire's design.

Event-based navigation
Switching to event-based navigation puts Kiosk where it needs to be in order to integrate react-common and gain the accessibility benefits to be had there. I introduced three main services to managed multi-modal input (tabbing, mousing, and gamepad(ing?)):

  • GamepadManager - modified the existing GamepadManager to emit events from gamepad input.
  • NavGrid - Figures which DOM element to focus based on directional navigation (gamepad or keyboard).
  • RectCache - NavGrid likes to query elements repeatedly for their DOMRect, so it made sense to cache them. This is purely for perf.

With these services in place, I reimplemented how UI components are navigated. No more direct polling of gamepad state to switch focus. Events emitted from gamepad activity will direct NavGrid to move focus in that direction. Components can hint NavGrid how they want to be treated when navigating, like specifying valid exit directions, or indicating they can be automatically focused. Components can override NavGrid if the generic approach just doesn't work for the use case. This is the case with the 3rd party carousel component we're using (swiper). It has it's own internal navigation system.

As part of this work I had to reimplement modal dialogs. To not stray too far from the goal of moving closer to compatibility with react-common, I stole react-common's FocusTrap and Modal components. Once react-common is pulled in, these will go away.

Aesthetic changes
I also made some changes to Kiosk's look & feel to align with Claire's design, and unfortunately included them in this PR.
If nobody is too opposed, I'd like to keep them here, and get them out to beta for group testing and feedback. pxt uploadtrg doesn't work with webapps at the moment, so I'm unsure of another way to easily publish these changes for testing.
Link to test target in comments below! Please give it a test and let me know what you love/hate.
image
image

Loose ends

  • There's a bug in the carousel with tab-based navigation when navigating away from the delete game button where the carousel gets all messed up. Seems like a bug in the control, but need to investigate it more.
  • There's a bug in the carousel with swipe-based navigation that results in the selected game mismatching what appears to be the current game in the carousel.
  • I'd like to avoid rewriting the carousel, but it's sort of on my chopping block at this point. I wrestled with it a lot.

@eanders-ms eanders-ms requested review from srietkerk and a team September 28, 2023 23:47
@eanders-ms
Copy link
Contributor Author

eanders-ms commented Sep 29, 2023

I was wrong about uploadtrg not supporting webapps! Here is a test build: https://arcade.makecode.com/app/e65a1da46ae1b3ef5f92dc000c36783fa5989d28-454ef2e16d--kiosk

See updated link below.

@eanders-ms
Copy link
Contributor Author

eanders-ms commented Sep 29, 2023

Copy link
Contributor

@srietkerk srietkerk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't made it through everything yet, but I wanted to get these comments out so I don't lose those thoughts.

@@ -127,13 +111,12 @@ const AddingGame: React.FC<IProps> = ({}) => {
</li>
</ol>
</div>

<div className="QRCodeHolder">{qrDivContent()}</div>
<div style={{ flexGrow: 1 }} />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new div adds a spacer between the instructions and the QR code that will grow to fill the available space, pushing them apart as the viewport gets wider and wider. I'm sure there's a pure CSS way to accomplish this, but this is what came to mind first.

return;
}
image.current.crossOrigin = "anonymous";
image.current.src = `https://makecode.com/api/${selectedGame.id}/thumb`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I just thought of that might be an improvement is to store the url for the thumbnail just in the game data that we already store in local storage. That way, we avoid building this url every time and we can just fetch the image based on selectedgame.thumbnail. I don't know if this would also mean that we can get rid of the dataUrlCache, just a suggestion.

Copy link
Contributor Author

@eanders-ms eanders-ms Sep 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a thumbnail member on GameData, but I get your meaning. Caching fetched thumbnails is a great idea.

dataUrlCache is a little different from a thumbnail cache. It caches an 80x60 rendering of the thumbnail, or the first frame of the thumbnail if it's a gif, as a base-64 encoded png. I have a TODO to apply the acrylic effect to this data rather than having CSS do it. So, dataUrlCache is (or will be) a cache of low-res, blurry, dark versions of thumbnails.

canvas.current.height = 60;
const ctx = canvas.current.getContext("2d");
if (ctx) {
// TODO: Apply blur and opacity filters to image here, not in CSS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we want to apply filters in JavaScript over CSS? I was under the impression that CSS would be faster at general styling things?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CSS filters suffer performance issues on low-end hardware. They need a GPU to perform well. Right now, every time we switch the background, CSS reapplies the acrylic effect to the image. I want to avoid this by applying it once and caching that result.

if (kiosk.kioskState === KioskState.PlayingGame) {
return "background-flat";
} else {
return "background-acrylic";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this addition. It adds a really nice polish to kiosk!!

const localSwiper = useRef<SwiperClass>();
const [userInitiatedTransition, setUserInitiatedTransition] =
React.useState(false);
const [pageInited, setPageInited] = React.useState(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite sure what "inited" is.. Is this to check initialization ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is me fighting with Swiper for who gets to decide which game is selected on component mount. I don't like it. This carousel is feature rich, but surprisingly difficult to control.

const deleteGame = () => {
const userAddedGames = Storage.getUserAddedGames();
const gameId = state.selectedGameId;
if (gameId && gameId in userAddedGames) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typically prefer to avoid in as it's got confusing / mildly unexpected semantics at times ("toString" in {} === true) -- prefer using userAddedGames.hasOwnProperty(gameId) for that reason but not a big deal either way

@@ -109,27 +41,20 @@ const MainMenu: React.FC<IProps> = ({}) => {
<div className="mainMenu">
<nav className="mainMenuTopBar">
<h1 className={`mainMenuHeader${lockedClassName}`}>
Select a Game
SELECT A GAME
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be better to have as a text-transform: uppercase in css, but can always clean that up when we actually add lf's

}
};

export const addGameToKioskAsync = async (
export const canthrow_addGameToKioskAsync = async (
Copy link
Member

@jwunderl jwunderl Sep 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kinda brings me back to java with throws keyword propagating upwards till it's handled 😄 (but in this sort of case really useful to know explicitly)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a pass to standardize how backend request failures are handled, but this one remains an "exception" to the pattern (harhar).

@eanders-ms eanders-ms merged commit b312c0c into master Oct 5, 2023
6 checks passed
@eanders-ms eanders-ms deleted the eanders-ms/kiosk-event-based-navigation branch October 5, 2023 16:56
eanders-ms added a commit that referenced this pull request Oct 24, 2023
* Kiosk: Support event-based navigation

* remove unused packages

* Add spacer to add game layout to keep it centered at wide resolutions

* Restore link border on scan qr page

* Shortened background transition time

* Better focus trapping. Fix tab nav on ScanQR page.

* pr feedback

* prettier
eanders-ms added a commit that referenced this pull request Oct 24, 2023
* Add `--no-webapps` gulp option (#9622)

* Add `--no-webapps' gulp option

* Fix function declaration inconsistency

* docs

* Move kiosk project to pxt repo (#9624)

* Move kiosk to pxt repo

* sync latest kiosk changes from arcade repo

* Move kiosk config files to arcade docs

* kiosk updates

* update GameDataUrl

* comment missing icon resources

* disable eslint, for now

* fix code scan warning

* Include kiosk build in npm package contents (#9631)

* Sync kiosk changes (#9645)

* Restore absolute url on kiosk qr code (#9660)

* Add prettier to kiosk (#9667)

* kiosk: increase gamepad polling frequency (#9668)

* Kiosk navigation improvements (#9669)

* Navigation improvements

* add return types

* sound effect tweak

* Add switch sound to Adding Game screen

* New UI effects for Kiosk, authored in Arcade (#9671)

* New UI effects authored in Arcade

* Update readme

* Clarify readme

* Moving kiosk sfx to arcade repo (#9672)

* Remove unused components from kiosk (#9678)

* disable app insights correlation headers (#9679)

* load pxtlib into kiosk (#9680)

* Made a few small navigation improvements (#9686)

* Kiosk: Use pxt.tickEvent (#9687)

* Kiosk: Use pxt.tickEvent

* remove localhost special case

* Refactor kiosk state management (#9689)

* kiosk: refactor add game polling (#9691)

* kiosk: refactor add game polling

* show toast on game delete

* poll for games even while playing a game

* Kiosk: Remove direct dom manipulation (#9692)

* Remove direct dom manipulation

* tweak adding game css

* prettier

* Removed unneeded css attribute

* update readme

* Remove unwanted memo dependency

* Kiosk: Support event-based navigation (#9700)

* Kiosk: Support event-based navigation

* remove unused packages

* Add spacer to add game layout to keep it centered at wide resolutions

* Restore link border on scan qr page

* Shortened background transition time

* Better focus trapping. Fix tab nav on ScanQR page.

* pr feedback

* prettier

* lf all the strings (#9710)

* kiosk: support gamepad dpad for navigation (#9712)

* Identify kiosk uwp app in telemetry (#9714)

* kiosk: setup react-common dependency (#9717)

* react-common: support for button children (#9719)

* kiosk: use button control from react common (#9720)

* css fix for "press start" label on safari (#9722)

* kiosk: download targetconfig.json at startup (#9723)

* kiosk: download targetconfig.json at startup

* update cli crowdin thing

* Update cli/cli.ts

Co-authored-by: Joey Wunderlich <[email protected]>

---------

Co-authored-by: Joey Wunderlich <[email protected]>

* Add `--noauth` option to `pxt serve` (#9725)

* kiosk: persist built game js in local storage (#9726)

* kiosk: persist built game js in local storage

* better null check

* kiosk: fixes for carousel touch nav, storage exceptions (#9731)

Pushing this through in time for testing today.

* fixes for carousel touch nav, storage exceptions

* don't save compiled js in local storage.

* Log gamepad type to telemetry (#9739)

* support setting color of "loading" text in run.html (#9738)

* fix for shoebox controller (#9737)

* kiosk: remap esc to the controller's back button (#9735)

* kiosk: fix for skipped letters when entering high score (#9734)

* kiosk: fix for skipping letters when entering high score

* whitespace

* kiosk: store built js in indexeddb, not local storage (#9736)

* kiosk: map controller Y to escapebutton functionality (#9741)

---------

Co-authored-by: Joey Wunderlich <[email protected]>
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