Skip to content

Conversation

@tceluzza
Copy link
Contributor

@tceluzza tceluzza commented Mar 6, 2022

This should fix the issue we were having with open rider spots. It makes 5 separate database queries and if one comes back empty, that rider spot is left blank on the whiteboard. It also no longer shows a radio number of 0 if the spot is set as reserved or OOS.
I also took the opportunity to add the favicon (pulled from rpiambulance/website since Jacob mentioned it was missing.

Copy link
Member

@ddbruce ddbruce left a comment

Choose a reason for hiding this comment

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

Just one question: the query as written produces the following output due to the <first initial. <last name> formatting we impose:

image

There's no dev deployment of headsup, so I can't test if your code functions the same way or not without merging to prod.

Also, I'll take this time to say that if a spot is open, we should display something better than "OUT OF SERVICE"...it should just be blank or grayed out or something, but that's for another PR.

@ddbruce ddbruce changed the base branch from dev to master March 10, 2022 23:43
@ddbruce
Copy link
Member

ddbruce commented Mar 10, 2022

Also I changed the base branch as dev doesn't really do anything for this repo. You'll have to reconcile some things.

@tceluzza
Copy link
Contributor Author

tceluzza commented Mar 10, 2022

Just one question: the query as written produces the following output due to the <first initial. formatting we impose

Yes, the new query returns the same format of name for OOS/Reserved positions:
image
image
The column name is obviously different since each position is getting queried separately, so it no longer lists it as rider1 or rider1rn anymore. However, I did change it so that when it actually displays on the Headsup page, it just shows as "OUT OF SERVICE" or "RESERVED" instead of ". OUT OF SERVICE" or ". RESERVED", and a vacant rider spot is left blank.

You'll have to reconcile some things.

What are you referring to? Everything looks good on my end except for the codeclimate check, but I don't think there's anything I can do there.

@ddbruce
Copy link
Member

ddbruce commented Mar 11, 2022

There's supposed to be a version check to ensure package.json is updated. I don't know why that's not showing right now. Also, I see this:

image

@tceluzza tceluzza requested a review from lramos15 as a code owner March 11, 2022 00:20
@tceluzza
Copy link
Contributor Author

I think I've fixed both of those. I couldn't see the out-of-date warning, but I've merged the changes into my fork and updated the version in package.json.

@tceluzza tceluzza requested a review from ddbruce March 11, 2022 00:50
Copy link
Member

@ddbruce ddbruce left a comment

Choose a reason for hiding this comment

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

lgtm

@ddbruce ddbruce merged commit 13c6a41 into techinems:master Mar 12, 2022
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