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

feat(example app): add modals page example 204 #250

Merged
merged 11 commits into from
Jun 15, 2020
Merged

feat(example app): add modals page example 204 #250

merged 11 commits into from
Jun 15, 2020

Conversation

eamahanna
Copy link
Contributor

Summary

This PR adds a modals page for the example app. On the modals page are two modals: one using redox and another that uses hooks.

Related Issues or PRs

closes #204

How To Test

  • Navigate to the react-uswds repo
  • Execute the following commands:
yarn build
cd example
yarn start
  • Navigate to the modals tab in the example app, and click on both buttons

Screenshots (optional)

Screen Shot 2020-06-11 at 11 54 11 AM

@eamahanna eamahanna requested a review from haworku June 11, 2020 19:55
haworku
haworku reviewed Jun 15, 2020
View reviewed changes
Copy link
Contributor

@haworku haworku left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good! Since this is meant to be a teaching tool, I made suggestions for how to match the behavior/implementation/naming between redux and hooks more closely. Sharing the TestModal code between both could help.

I think these types of changes would make it easier to compare approaches.

example/src/pages/Modals.tsx Outdated Show resolved Hide resolved
example/src/pages/Modals.tsx Show resolved Hide resolved
example/src/pages/Modals.tsx Outdated Show resolved Hide resolved
example/src/pages/Modals.tsx Show resolved Hide resolved
example/src/redux/reducer.test.ts Outdated Show resolved Hide resolved
example/src/redux/types.ts Outdated Show resolved Hide resolved
src/components/Modal/Modal.tsx Outdated Show resolved Hide resolved
@haworku
Copy link
Contributor

haworku commented Jun 15, 2020

@eamahanna General comment - If its not too messy could you point this off @suzubara branch? If we merge this PR alone, as written right now, it will bring in all of that work. Otherwise let's wait till that work merges to bring this in.

@eamahanna eamahanna requested a review from haworku June 15, 2020 17:35
@trussworks-infra-zz
Copy link

trussworks-infra-zz commented Jun 15, 2020 •
edited
Loading

Warnings
⚠️ This PR does not include changes to storybook, even though it affects component code.

Generated by 🚫 dangerJS against 21f2b71

haworku
haworku approved these changes Jun 15, 2020
View reviewed changes
Copy link
Contributor

@haworku haworku left a comment

Choose a reason for hiding this comment

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

Thank you!! One small comment. Also making sure you saw this #250 (comment). If this doesn't resolve before you have to offboard you can assign me to the issue and I can make sure it gets merged 😄

example/src/pages/Modals.tsx Outdated Show resolved Hide resolved
@eamahanna eamahanna changed the title Em example app modal 204 feat(example app): add modals page example 204 Jun 15, 2020
@eamahanna eamahanna merged commit 1054469 into develop Jun 15, 2020
@eamahanna eamahanna deleted the em-example-app-modal-204 branch June 15, 2020 18:40
haworku added a commit that referenced this pull request Jun 15, 2020
@haworku
Squashed commit of the following:
47f4462 
commit 1054469
Author: Emily Mahanna <56279459+eamahanna@users.noreply.github.com>
Date:   Mon Jun 15 10:40:45 2020 -0800

    feat(example app): add modals page example 204 (#250)

    * WIP Set up basic layout, router

    * Switch to react-router, add placeholder content

    * feat(example app): add modals example page

    * feat(example app): add redux for modals

    * hook up modals page

    * feat(example app): add hook modal

    * fix(modals page): fixes from PR review

    * fix(modals page): pr update

    * fix(modals page): remove comment

    * fix(modals page): add import back in

    Co-authored-by: Suzanne Rozier <suz@truss.works>

commit 832decc
Author: Suzanne Rozier <suz@truss.works>
Date:   Mon Jun 15 13:00:03 2020 -0500

    docs(example-app): Example app UI #202 (#237)

    * WIP Set up basic layout, router

    * Switch to react-router, add placeholder content

    * Add docs and commands for running the example app
@haworku haworku mentioned this pull request Jun 29, 2020
Merged
haworku pushed a commit that referenced this pull request Jun 29, 2020
@eamahanna @haworku
feat(example app): add modals page example 204 (#250)
482a0bb 
* WIP Set up basic layout, router

* Switch to react-router, add placeholder content

* feat(example app): add modals example page

* feat(example app): add redux for modals

* hook up modals page

* feat(example app): add hook modal

* fix(modals page): fixes from PR review

* fix(modals page): pr update

* fix(modals page): remove comment

* fix(modals page): add import back in

Co-authored-by: Suzanne Rozier <suz@truss.works>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@haworku haworku haworku approved these changes

@gidjin gidjin Awaiting requested review from gidjin

@sarboc sarboc Awaiting requested review from sarboc

@suzubara suzubara Awaiting requested review from suzubara

@tinyels tinyels Awaiting requested review from tinyels

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

Successfully merging this pull request may close these issues.

[feat] Add modal example to example app
4 participants
@eamahanna @haworku @trussworks-infra-zz @suzubara