-
Notifications
You must be signed in to change notification settings - Fork 707
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
Rewrite the Adagrams demo app using Ink #38
Open
mmcknett
wants to merge
75
commits into
AdaGold:main
Choose a base branch
from
mmcknett:mm-ink-demo-app
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mmcknett
force-pushed
the
mm-ink-demo-app
branch
from
July 13, 2022 07:34
732db8d
to
622dd88
Compare
Also move some existing state to it.
The menu to accept configuration and move on is not yet present, but there is state for the three options and the ability to update them.
Player entry was briefly flashing 1 + the number of players after entering the last player's name.
Ink complains the app has a memory leak if you change state after dispatching an action that unmounts the component.
This is a more common name for this type of function.
This includes a flag in the reducer that lets you start the game without setup.
The menu was cumbersome without callbacks, so I made the MenuEntry's second parameter optionally be a callback instead of a string.
Any other action resets the error state.
The win screen had a bunch of logic for calculating scores using Adagrams and the game state. This is similar to what redux calls "selectors". Extract those "selectors" into a class so that additional logic can be added. The extraction was done under test, but the tests rely on Adagrams working correctly, so I'm not merging them as-is.
Not only was it sketchy, React doesn't allow it and throws an error.
Proxy default values were not all working. The Real functions themselves were defined, they were just returning `undefined`.
mmcknett
force-pushed
the
mm-ink-demo-app
branch
from
July 13, 2022 07:44
622dd88
to
4e8ed0f
Compare
- Explain Ink a little more - Document Screens, with initial wireframes and an MVC exploration - Document Timer - Copyediting
Reading screens first probably makes more sense.
I also added a bunch of discussion on MVC, then put the whole section behind a collapsible TL;DR.
I used the term "middleware" incorrectly. Middleware is a higher-order function applied to the `dispatch` function. The structures I had called "middleware" in the documentation are actually higher-order reducers. They modify the reducer, but still guarantee a state is returned and do not produce side-effects. Middleware wrapping the dispatcher allows for side-effects and behavior that does not produce state modifications.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adagrams demo game rewrite
Motivation and Architecture
This PR is a rewrite of the Adagrams demo-game. It includes an Architectural Decision Record explaining the motivation and details of the decision to rewrite. If this PR is accepted and merged, consider that ADR also accepted.
The new application is written with Ink, which allows you to build CLI applications with React. Several prominent CLI tools, including
jest
, depend on this framework. The application's architecture is documented in README files in its folders, and is intended to walk inquisitive students through the organization and design of the app, in an attempt to fulfill the goals of the ADR.Testing
You can test the branch locally. If you've already cloned the repo, you can add my fork as a remote, install, and run:
The unit tests should pass except for adagrams, since that isn't implemented:
yarn test
You should be able to test the demo with a working Adagrams solution by merging
c17/solution
into it. Just don't push the merge commit. If this PR is accepted, I will rebase the solutions ontomain
.PR work tracking
Items remaining prior to PR publish:
Also want to investigate whether babel is still necessary for transpilation (e.g. of the js modules)current tests were all manual; I was having trouble thinking about how to test Ink appsstdin.write
calls. I was able to generally work around the issue by rerendering after any changes to the React tree could be expected, but it seems likely there is either some trouble between Ink and recent versions of React or that I missed some necessary testing setup.main