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

88 table view #104

Merged
merged 7 commits into from
Aug 1, 2020
Merged

Conversation

allthesignals
Copy link
Contributor

Addresses #88

Adds a table to display deliveries along with hyperlink. Does not paginate, but prevents table row overflow.

image

Next steps are to hook up the map so that rows highlight on hover.

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.

I have some comments but overall this looks dope! It's going to be really useful as we see increased deliveries from the temporary phone line closure to clear our queue.

Per the ticket #88, can you also add more fields as posted in #community_needs? The more fleshed out the table is, the easier it is for volunteers to decide what they can pick up. This includes household size, language, description, etc. which are particularly important for deciding how big a job will be.

All of this data isn't in the payload for the map but should be pretty easy to add on from the API. This can happen in a separate PR as well if you want to keep this one small!

Thank you for your contribution!

It's easy to run the app, but some features require additional configuration.

### Mapbox API Key
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be fetched from our staging heroku as well. folks can reach out for access in #wg_tech

<TableHead>
<TableRow>
<TableCell>Code</TableCell>
<TableCell align="right">Cross Street #1</TableCell>
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should be using str translations here as well. See README for details on how to add i18n.

Copy link
Contributor

Choose a reason for hiding this comment

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

These strings in particular might already exist in the webapp namespace!

@allthesignals
Copy link
Contributor Author

I have some comments but overall this looks dope! It's going to be really useful as we see increased deliveries from the temporary phone line closure to clear our queue.

Per the ticket #88, can you also add more fields as posted in #community_needs? The more fleshed out the table is, the easier it is for volunteers to decide what they can pick up. This includes household size, language, description, etc. which are particularly important for deciding how big a job will be.

All of this data isn't in the payload for the map but should be pretty easy to add on from the API. This can happen in a separate PR as well if you want to keep this one small!

Thank you for your contribution!

Thanks, I'll address all of these ASAP!

@mjmaurer
Copy link
Contributor

mjmaurer commented Jun 26, 2020

I have a bigger ask that would be SUPER useful. would it be possible to sort open requests by time since the slack post? (and also display the lapsed time)

The message ts field which is used as a thread ID is also a Unix timestamp (https://www.unixtimestamp.com/index.php), so there wouldn't need to be any extra network calls

This would be a good supplement to #63

@allthesignals
Copy link
Contributor Author

I have a bigger ask that would be SUPER useful. would it be possible to sort open requests by time since the slack post? (and also display the lapsed time)

The message ts field which is used as a thread ID is also a Unix timestamp (https://www.unixtimestamp.com/index.php), so there wouldn't need to be any extra network calls

This would be a good supplement to #63

You bet! I think I can work on this tomorrow if that's alright. I realize the table needs a lot more work before it'll be useful.

@piratefsh
Copy link
Collaborator

piratefsh commented Jun 26, 2020 via email

@piratefsh
Copy link
Collaborator

piratefsh commented Jun 26, 2020 via email

@mjmaurer
Copy link
Contributor

Yup feel free to do a different PR. Does this PR have an associated issue that I'm missing?

@allthesignals
Copy link
Contributor Author

#88

@allthesignals
Copy link
Contributor Author

@mjmaurer I'm currently out of town dealing with a family matter — if someone wants to finish this before I am able to get back, that's totally fine

@mjmaurer
Copy link
Contributor

mjmaurer commented Jul 3, 2020

any eta? I might be able to take this up and I'll try to make sure the commits remain yours

@piratefsh
Copy link
Collaborator

In the interest of moving forward with this feature, imma propose that we address existing issues from comments above, assess if it is safe and helpful to go to prod and iterate from there. wdyt?

thanks @allthesignals for starting the work. I'm happy to take over this PR from here if no one else wants to.

@mjmaurer
Copy link
Contributor

mjmaurer commented Jul 9, 2020

makes sense to me!

@allthesignals
Copy link
Contributor Author

Sorry @mjmaurer @piratefsh , I'm still away dealing with some issues. I could look at it late next week at the earliest. I have no problem with anyone who wants to take it over.

@allthesignals
Copy link
Contributor Author

Looking at this again. Rebased with changes, a few merge conflicts.

To sort by slack post, I'll need to figure out how to get that data in.

@allthesignals
Copy link
Contributor Author

Oh woops let me get the str changes in

Includes changes to the grid system so that the table and explainer text can show in the same view.
Replaces the ts-react-json-table dependency with material-ui, which is already installed and available.
Copy link
Contributor

@mjmaurer mjmaurer left a comment

Choose a reason for hiding this comment

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

Only one comment. thanks so much!

{formattedRows.map((row) => (
<TableRow key={row.Code}>
<TableCell component="th" scope="row">
{row.Code}
Copy link
Contributor

Choose a reason for hiding this comment

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

can you replace these row field strings with those in lib/airtable/tables/requests?

Copy link
Contributor Author

@allthesignals allthesignals Jul 23, 2020

Choose a reason for hiding this comment

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

@mjmaurer sure! But how do I import ~airtable/tables/requests into the react app?

If I follow, we're using these row field strings to avoid weird property names ("Cross Street #1").

import Requests from "~airtable/tables/requests"; throws

Module not found: Can't resolve '~airtable/tables/requests' in '/Users/wmattgardner/mutual-aid-app/src/webapp/components'

Importing with relative path throws errors about the dependencies of the thing being imported.

Any ideas? Am I totally missing something? Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

hey @alllthesignals! Michael is out atm, Sherminn here. I think we probably don't have path aliasing set up for the react app, so you might just have to use relative path e.g. ../../lib/airtable. Since this is non-blocking, plus we have other places in the webapp where we need to fix this, im going to go ahead and merge this first and follow up with a new issue to handle hardcoding of airtable field keys.

This table view will be important for the upcoming work for #122, where volunteers can pick up delivery straight from the map. thank you for your contribution!

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.

approved with ticket here to follow up on hardcoded strings here: #124

this will unblock SMS delivery pickup work! #122

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.

3 participants