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

[SMS delivery pickup] Build basic UI for accepting delivery #132

Merged
merged 20 commits into from
Oct 2, 2020
Merged

[SMS delivery pickup] Build basic UI for accepting delivery #132

merged 20 commits into from
Oct 2, 2020

Conversation

ifearcompilererrors
Copy link
Collaborator

@ifearcompilererrors ifearcompilererrors commented Aug 5, 2020 •
edited
Loading

TODO:

  • ask intake for dialog copy
  • phone number field
  • phone number validation
  • "Yes" state
  • finish state
  • "No I need more info" state
  • hide claim delivery button when sms_pickup=true param is present

closes #122
closes #133

@ifearcompilererrors ifearcompilererrors force-pushed the feature/ui-claim-delivery branch 2 times, most recently from 87b829e to a8dafaa Compare August 7, 2020 17:42
@ifearcompilererrors ifearcompilererrors force-pushed the feature/ui-claim-delivery branch from a8dafaa to 3f4c59a Compare August 7, 2020 17:58
@piratefsh
Copy link
Collaborator

piratefsh commented Aug 10, 2020 •
edited
Loading

hey! i just took a quick look and everything looks good so far! i see that you introduced a context and there's some weird circular dependency happening. i'll also be introducing another context in a future PR that's for a different flow. i'm thinking we can just put contexts in webapp/components/contexts, that way it can be imported without circular dependency + reduce merge conflicts in the future. any thoughts? whoops ignore all that, i see you already did that!

@piratefsh
Copy link
Collaborator

i'll update my context to be in webapp/context so we don't end up with duplicated dirs!

@ifearcompilererrors
Copy link
Collaborator Author

@piratefsh woohoo! thanks for taking a look. Excited for more context wowow. The dialog UI probably looks a little rough without copy, but hoping it's a step in the right direction!

@ifearcompilererrors ifearcompilererrors force-pushed the feature/ui-claim-delivery branch 3 times, most recently from dc7fb8b to 0b82586 Compare August 16, 2020 20:10
@ifearcompilererrors ifearcompilererrors marked this pull request as ready for review August 16, 2020 20:12
ifearcompilererrors
ifearcompilererrors commented Aug 16, 2020
View reviewed changes
src/webapp/components/DeliveryDialog/InstructionsScreen.js Outdated
li: ListItem,
};

const Instructions = () => {
Copy link
Collaborator Author

@ifearcompilererrors ifearcompilererrors Aug 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

lmk if i over-engineered this 👀 happy to walk through how this works and will add helpful comments before merging if we decide to keep this

Copy link
Collaborator

Choose a reason for hiding this comment

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

not at all! having instructions in a separate JSON file will make it easier for future copy updates. i only worry about doing this alongside the use of i18n might be confusing to other mutual aids that forked from us, so we should make sure to document why we're doing this (i imagine to allow for flexibility in structuring the content).

piratefsh
piratefsh approved these changes Aug 16, 2020
View reviewed changes
Copy link
Collaborator

@piratefsh piratefsh left a comment

Choose a reason for hiding this comment

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

lgtm! looks like a solid start! i had one comment re: documenting the use of the rich-text JSON style of managing copy.

in terms of UX, i didn't add this as a requirement (my bad!) but we should also handle the following:

  • on success, indicate that the request had been assigned to you. instead of straight up removing the request, im thinking we can just disable the claim delivery button on that popup.
  • error state in FinishStep, what if that request failed? we will need an error state to retry or contact #wg_tech on Slack.

These can be fixed in a future PR, not blocking for this since this feature is hidden anyway

Also we may have some merge conflicts from my recent PR, happy to help fix those once I merge to master: #137

src/webapp/components/DeliveryDialog/InstructionsScreen.js Outdated
li: ListItem,
};

const Instructions = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

not at all! having instructions in a separate JSON file will make it easier for future copy updates. i only worry about doing this alongside the use of i18n might be confusing to other mutual aids that forked from us, so we should make sure to document why we're doing this (i imagine to allow for flexibility in structuring the content).

src/webapp/components/DeliveryDialog/InstructionsScreen.js Outdated
import DialogContent from "@material-ui/core/DialogContent";
import DialogActions from "@material-ui/core/DialogActions";
import Button from "@material-ui/core/Button";
import instrutions from "./instructions.json";
Copy link
Collaborator

Choose a reason for hiding this comment

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

small nit: typo here

@ifearcompilererrors ifearcompilererrors force-pushed the feature/ui-claim-delivery branch from 0b82586 to 473935d Compare August 20, 2020 02:22
@ifearcompilererrors ifearcompilererrors force-pushed the feature/ui-claim-delivery branch from 473935d to 5d780c4 Compare August 20, 2020 02:32
@ifearcompilererrors ifearcompilererrors force-pushed the feature/ui-claim-delivery branch from df823ee to c6761ca Compare August 20, 2020 13:15
@ifearcompilererrors ifearcompilererrors mentioned this pull request Aug 23, 2020
Merged
2 tasks
ifearcompilererrors and others added 4 commits September 26, 2020 11:20
@ifearcompilererrors
[SMS delivery pickup] Add endpoint that fires a request to Twilio (#143)
150ed5c 
* implement assign delivery enddpoint

* add error screen in delivery dialog; misc cleanup and bug fixes

* better error messages; restrict phone number form to US country code only

* refetch data after delivery is successfully claimed; claim delivery before texting; update find volunteer by phone logic

* better client error messages

* Update twilio endpoint

* Cleanup copy and refactor out claim delivery button to reusable component

* add loading state to deliver dialog

* add intake volunteer to twilio request body

* use volunteer field name string instead of map

* Use TWILIO_SMS_DELIVERY_ENPOINT as env var

* Add env var to .env.example + linting fix

Co-authored-by: Angelique De Castro <angelique.decastro@nytimes.com>
Co-authored-by: Sher Minn Chong <sherminn.chong@gmail.com>
Merge branch 'master' into feature/ui-claim-delivery
488a878
Move deliverry button to description in table
6834041
Fix linting
3317d9b
@piratefsh piratefsh mentioned this pull request Sep 30, 2020
Closed
@piratefsh piratefsh merged commit ec5f942 into master Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@piratefsh piratefsh piratefsh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@ifearcompilererrors @piratefsh