-
Notifications
You must be signed in to change notification settings - Fork 7
Conversation
minor renaming minor renaming minor renaming
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks 🎉 to me, just a note on the API response structure
"zip": "78744", | ||
"country": "United States" | ||
}, | ||
"hours": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a slightly different structure for contacts & locations currently cityofaustin/joplin#39 (comment). Some goofiness is there because of the default Django rest framework structure, but I separated contact & location. Not sure if that's the right way to model it or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made some comments here -- cityofaustin/joplin#39
but basically, since there will be multiple locations and points of contact, I think there will be cases where these two should be associated when shown to residents. For ex, contact info for Austin Resource Recovery main drop off location vs Treehouse where classes are hosted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
text={service.title} | ||
/> | ||
</div> | ||
) | ||
} | ||
</div> | ||
<a className="coa-section__link" href="#">See a Full List of 311 Services</a> | ||
<a className="coa-section__link" href="http://311.austintexas.gov/reports/list_services">See a Full List of 311 Services</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we do target="_blank"
on these since we link them to a separate site? Or since it is the same domain, maybe not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not 100% sure, I can ask Celine what her thoughts are
@@ -22,7 +23,7 @@ class Hours extends Component { | |||
hours.map((hour, index) => | |||
<tr key={index}> | |||
<th scope="row">{hour.day_of_week}</th> | |||
<td>{hour.start_time} - {hour.end_time}</td> | |||
<td>{moment(hour.start_time, "HH:mm:ss").format('h:mm A')} - {moment(hour.end_time, "HH:mm:ss").format('h:mm A')}</td> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
update 311 links with outbound urls
update contact section to handle multiple contacts (rely on stub data as Joplin model isn't updated)
update related service links based on Celine's mocks closes #41