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.

Sign up for GitHub

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

Jump to bottom

Kiosk: Remove direct dom manipulation #9692

Merged
merged 6 commits into from
Sep 21, 2023
Merged

Kiosk: Remove direct dom manipulation #9692

merged 6 commits into from
Sep 21, 2023

Conversation

eanders-ms
Copy link
Contributor

  • Move simulator iframe to a React component
  • Removed direct DOM manipulation
  • Move footer into React, reduced bottom margin
  • Some compensating css tweaks

@eanders-ms eanders-ms requested review from srietkerk and a team September 21, 2023 18:21
srietkerk
srietkerk reviewed Sep 21, 2023
View reviewed changes
kiosk/src/Components/PlayingGame.tsx
const playUrlBase = `${configData.PlayUrlRoot}?id=${gameId}&hideSimButtons=1&noFooter=1&single=1&fullscreen=1&autofocus=1`;
const playQueryParam = kiosk.builtGamesCache[gameId]
? "&server=1"
: "&sendBuilt=1";
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be misunderstanding this, but would we want to change the order for this ternary operator? If there is a game in the builtGamesCache, we would want the sendBuilt=1 flag, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You might be correct! Code is unchanged from the original: https://github.com/microsoft/pxt-arcade/blob/0b1189cbc8abc015854cc940e63f6ae9dc192de4/kiosk/src/Models/Kiosk.ts#L343

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'd prefer not to change it in this PR, but will file an issue to investigate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, interesting.. Yeah, this might be a bug. If we don't see performance improvements for subsequent game launches, then the order is wrong. If we do see those load time improvements, then there's something that I'm reading incorrectly. Like you said, something to investigate.

kiosk/src/Components/PlayingGame.tsx Outdated
: "&sendBuilt=1";
return playUrlBase + playQueryParam;
}
}, [gameId, kiosk.builtGamesCache]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why we want the kiosk.builtGamesCache to be a dependency for the useMemo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. We probably don't. We wouldn't want to re-launch the game if it changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

srietkerk
srietkerk approved these changes Sep 21, 2023
View reviewed changes
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.

Looks good!

@eanders-ms eanders-ms merged commit 5b4bd1c into master Sep 21, 2023
6 checks passed
@eanders-ms eanders-ms deleted the eanders/kiosk-tidy-components branch September 21, 2023 22:46
eanders-ms added a commit that referenced this pull request Oct 24, 2023
@eanders-ms
Kiosk: Remove direct dom manipulation (#9692)
3747ea3 
* Remove direct dom manipulation

* tweak adding game css

* prettier

* Removed unneeded css attribute

* update readme

* Remove unwanted memo dependency
eanders-ms added a commit that referenced this pull request Oct 24, 2023
@eanders-ms @jwunderl
[stable8.5] cherry-pick all kiosk commits (#9743)
8607876 
* 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
Reviewers

@srietkerk srietkerk srietkerk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@eanders-ms @srietkerk