search

xunhuang / covid-19

MIT License
17 stars 16 forks source link

Adding ask to user for location & adding button to ROW search box #122

Closed tumbleshack closed 4 years ago

tumbleshack commented 4 years ago

TODO: Add function to find the user's country code based on their coordinates. If it's the US, then use it to find their county.

Contributes to #121 and closes #100

aschleck commented 4 years ago

Cool! Just curious, are you interested in getting a review for ways to improve the code or are you mostly interested in submitting quickly? I'd be happy to give you thoughtful code reviews if you actually want them (I recall somewhere you said you wanted to use this project as a chance to get better at React/developing), but I think we're mostly just yolo submitting to keep velocity high and I don't want to unilaterally change that so definitely not trying to force this on you.

Full disclosure, I didn't know anything about React prior to joining this project so am definitely not a React expert. Also I am nitpicky.

xunhuang commented 4 years ago

McKay is a student at College, I'd definitely think thoughtful feedback is better than velocity. He has hope of learning from you, unlike me keep repeating same mistakes. :)

On Sat, May 9, 2020 at 8:05 PM April Schleck notifications@github.com wrote:

Cool! Just curious, are you interested in getting a review for ways to improve the code or are you mostly interested in submitting quickly? I'd be happy to give you thoughtful code reviews if you actually want them (I recall somewhere you said you wanted to use this project as a chance to get better at React/developing), but I think we're mostly just yolo submitting to keep velocity high and I don't want to unilaterally change that so definitely not trying to force this on you.

Full disclosure, I didn't know anything about React prior to joining this project so am definitely not a React expert.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/xunhuang/covid-19/pull/122#issuecomment-626265832, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFSGF4D2LYUQYU35VVVFDLRQYKXXANCNFSM4M5BGGJA .

tumbleshack commented 4 years ago

Yes! I would love some thoughtful code reviews if you’re willing to give them. I’m out to learn! I’ll make this non-draft so it’s open to comments.

tumbleshack commented 4 years ago

I'm sorry for turning nitpicks to 11 too

Not a problem at all. I tremendously appreciate the time you put into the review! Thank you!

tumbleshack commented 4 years ago

Used the SearchIndexComponent to implement ROW's GeoLocate button... is this too hacky? I considered a few ways to do it and landed on this one for simplicity. Definitely open to critiques & suggestions.

xunhuang commented 4 years ago

@aschleck, I'm not following this PR very closely, feel free to squash and merge when you feel it's good enough. :)

@tumbleshack , You think FB's reviewers are tough?

xunhuang commented 4 years ago

thank you guys so much for working through this PR. I have learned a lot from this too.

tumbleshack commented 4 years ago

My last comments were minor so happy to approve, will wait to squash in case you want to add more commits. Looks great on my phone and computer!

Okay I can't resist one more thought: generally I sort the CSS selectors alphabetically because I was told to: https://google.github.io/styleguide/htmlcssguide.html#Declaration_Order . I'm not going to block this on that, but it's impossible for me to tell how you're organizing your selectors so I'd encourage you to do it. It makes things look cleaner anyway.

Haha much better! I was roughly trying to organize them by purpose (sizing, extra spacing, color, font, etc) but alphabetical is much easier for finding them later... lol my brainfart

Final comments addressed in #124