Cleans up details a bit. More compact, order more relevant. Fixes #35

hurricane-response / response-theme

Jekyll theme scaffold for deploying CFA disaster response/shelter tracking map

MIT License

2 stars 8 forks source link

Cleans up details a bit. More compact, order more relevant. Fixes #35 #52

Closed plocket closed 5 years ago

plocket commented 5 years ago

Description

Fixes #35

Shelter details should now look like this:

address
directions
phone icon, phone number
'Pets' label, pets details
Notes label
Notes details

List of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

[x] This pull request relates to a ticket.
[x] This code is tested.
[x] I want my code added to the response theme.

plocket commented 5 years ago

@miklb : What more does this need before it can be merged?

plocket commented 5 years ago

@omnilord : I can't add reviewers, so I'll just ping you here for a review.

plocket commented 5 years ago

@omnilord : done as far as I can tell

omnilord commented 5 years ago

Looks decent. Removing the directions arrow may be a bit confusing since it is a ubiquitous visual cue for "get me directions to this location." Thoughts on making it more stylistically a better fit kind of like with the phone icon?

plocket commented 5 years ago

The arrow doesn't help during the day because it's white. I can change the color. Or show me the phone icon and I can try. Does this theme have fontawesome?

miklb commented 5 years ago

No, it uses SVGs. I have an undocumented gulp task to inline svgs Previously they were just hard coded.

miklb commented 5 years ago

I might be thinking of another project though, I’ve done similar. Definitely has always been svg

plocket commented 5 years ago

Oh, shoot, I was thinking of a different PR from earlier. I can restore this one.

plocket commented 5 years ago

Added back in. I also noticed it was a little hard to tell that the links were links. It's hard to see the blue color. At least, it is for me. I added underlines. Let me know if that's a problem.

miklb commented 5 years ago

looks good to me. Probably want a new issue for the null on notes unless someone has a quick solution to add to this PR.

plocket commented 5 years ago

@miklb : If you think it's related enough to this PR, I can take a stab. Let me know if it'd just be better to merge what we have so other merges can move forward.

miklb commented 5 years ago

Not a blocker on my end. If you wouldn't mind opening the issue that would be helpful to remember. I'm working through some organizing now.

plocket commented 5 years ago

@miklb : It looks like it was maybe taken care of? I'm getting 'Unknown' for a lot of these and not seeing any nulls.

@miklb , @omnilord : Is this ready to merge?

plocket commented 5 years ago

@miklb : Nvm, just found one I can test with!

plocket commented 5 years ago

@miklb : I've figured out a way. I'll make an issue and then a PR once this other stuff has been merged.

plocket commented 5 years ago

@omnilord : Done. Didn't know it didn't happen before and not sure why... a puzzle for another time.