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

WIP: Makes download/install options available on frontpage, adds preflight checklist. #44

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

FrenjaminBanklin
Copy link
Contributor

Closes #43.

Cleans up some redundant calls to css/js assets in MDWK pages.

Makes the Download/Install window available on the MWDK splash screen.

Adds a series of checks to the Download/Install window that verify presence/correctness of critical files and assets to the widget before allowing it to be downloaded or installed to Materia.

…dancy and enable the 'Download Package' button on the splash screen, started working out backend logic for making preflight checks.
…o the output from the preflight checklist. Covered all checks so far.
@FrenjaminBanklin FrenjaminBanklin requested a review from a team March 27, 2019 19:34
Copy link
Member

@iturgeon iturgeon left a comment

Choose a reason for hiding this comment

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

My first attempt to use it failed right away. Sort it out's score screen is scoreScreen not scorescreen and preflights errored out because it couldn't find it. I don't think we can be strict about failures for things that are not guaranteed to be there.

I'll try a couple ideas and report back

@iturgeon iturgeon changed the base branch from master to dev/2.4.0 May 29, 2019 02:27
Copy link
Member

@iturgeon iturgeon left a comment

Choose a reason for hiding this comment

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

Currently this looks like it's making a lot of assumptions. We probably can't make assumptions or enforce standard locations of these files - so this may be difficult to deal with.

Ideas:

  • A way to disable some of these checks for a widget
  • Use a temporary localstorage to disable some checks
  • Don't prevent the download / install buttons from displaying when there are errors
  • Make searching for the expected functions more robust (fiddly)
  • Add options to the install.yaml to hint where to look for the things we're testing


//check score screen callbacks
if(install.score.score_screen) {
const scoreScreen = getFileFromWebpack('scorescreen.js').toString()
Copy link
Member

Choose a reason for hiding this comment

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

scorescreen.js won't work in all situations. In fact it's super hard to figure out what js file to load. It can be just about anything. Perhaps we can't automate it (or maybe we need to do a little more guessing or detection).

For instance, Crossword uses scoreScreen.js.

Crossword's Install.yaml:

score:
  is_scorable: Yes
  score_module: Crossword
  score_screen: scoreScreen.html

which would then lead us to the js script loaded in the html...

Crossword's scoreScreen.html:

<!DOCTYPE html>
<html>
	<head>
		<title>Crossword</title>
		<!-- ... -->
		<script src="scoreScreen.js" data-embed></script>
	</head>

That said, it may not even be in the head - and it may not be the only js file loaded.

const playerSaveMatch = player.match(/Materia.Engine.start/g)
if(!playerSaveMatch || playerSaveMatch > 1) {
status.playerCallback = 'fail'
action.playerCallback = "EngineCore 'start' method missing or called more than once"
Copy link
Member

Choose a reason for hiding this comment

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

This fails on Crossword - it's called in the html file in a script tag, so it's not discovered by this script.

@iturgeon
Copy link
Member

We talked a bit about this today - we're thinking about allowing devs to place options in the package.json under the materia key.

{
  "name": "@ucfopen/crossword",
  "materia": {
    "cleanName": "crossword"
    "files": {
       "player": "src/player.js"
       ....
     }
  },

@FrenjaminBanklin FrenjaminBanklin changed the title Makes download/install options available on frontpage, adds preflight checklist. WIP: Makes download/install options available on frontpage, adds preflight checklist. Jun 11, 2019
@clpetersonucf clpetersonucf changed the base branch from dev/2.4.0 to master July 31, 2019 14:52
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.

Add preflight checks to help new widget authors
2 participants