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

Hook refactor prescreen-1d #251

Merged
merged 2 commits into from
Dec 16, 2020
Merged

Conversation

dylanesque
Copy link
Collaborator

Refactored this component to use a functional component rather than a class component.

@thadk thadk changed the base branch from develop to line8line40 December 6, 2020 06:36
@thadk
Copy link
Member

thadk commented Dec 6, 2020

@dylanesque Nice job on this first pass! I haven't had a chance to run it yet but do have one early comment on code review:

I looked closer at the example I suggested for you from https://github.com/codeforboston/windfall-elimination/blob/develop/src/pages/screen-2a.tsx and I realized that one house-cleaning tweak relevant here got lost in the mob programming. Thanks for patience.

You already cleaned up some of it (i.e. FunctionComponent). Basically because we're now able to have the useUserState() and useUserStateActions() in the main function to access the two contexts, we no longer need the class component prop wrapper or its fancy typed interface syntax for the props.

If I recall right, I think our lost solution in the mob programming session for screen-2a.tsx was just to remove the two context-based props (userState, userStateActions) and all the uses of Prescreen1dProps interface. You might need to try a few things though if my memory is failing me. Feel free to bring it on Tuesday if it doesn't work as easily as I think.

If you get a chance, in another PR to develop branch, feel free to make these changes in screen-2a.tsx as well, tidying up as necessary, so we have two clean examples of hooks pages once both merged.

@thadk
Copy link
Member

thadk commented Dec 6, 2020

This is significant progress on #135

@dylanesque
Copy link
Collaborator Author

That all tracks:: I tested it briefly and it compiled just fine and didn't throw any errors, but didn't really stress-test the changed section. I'll implement your notes, and take a closer look.

@thadk thadk merged commit 2dc4220 into codeforboston:line8line40 Dec 16, 2020
@thadk
Copy link
Member

thadk commented Feb 5, 2021

@dylanesque Thanks again, Mike. Your code is now live but behind the feature hook until we finish that page. I will use it as an example for other people to convert pages to hooks.

Anyway, your code shows when you run develop branch with GATSBY_SHOW_FUTURE_EARNINGS_PAGE=true npm start

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.

2 participants