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

Add a button to change the current address #7 #20

Merged
merged 4 commits into from
Oct 5, 2020

Conversation

urish
Copy link
Contributor

@urish urish commented Oct 4, 2020

Fixes #7, #16. If you want, I can also style the button (just let me know)

@guytepper
Copy link
Owner

Hi, thanks! Much better solution than a refresh :)

Any reason you decided to place it above 'far protests' and not on top?

I think it's better to be placed in App.js under <SiteMessage>, since this button isn't tightly related to ProtestList (which we might use in other places soon).
It also prevents the padding issue from the container here:

Screen Shot 2020-10-05 at 6 59 02

You can use the Button component for styles.
If you have time, it might be wise to use some kind of prop to pass to it's styled-components method for making it take 100% width (it's set to a static 300px right now). Otherwise just use style={{ width: '100%' }} on it.
Let's use #3C4F76 for the button background.

@urish
Copy link
Contributor Author

urish commented Oct 5, 2020

Any reason you decided to place it above 'far protests' and not on top?

No strong opinion about this, but it seems more logical from the perspective of the user to first scan the list of near-by protests before deciding on an address change. Should I change it and move the button on top (an to App.js)?

You can use the Button component for styles.

👍

@guytepper
Copy link
Owner

Yes, please change it to be on top :-)

@urish
Copy link
Contributor Author

urish commented Oct 5, 2020

Done! for now, I kept the context change in as it seems to be useful for other PRs (e.g. #29), but if you wish I can split it into a separate PR as well

@vercel
Copy link

vercel bot commented Oct 5, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/1km/1km/eaebf0lug
✅ Preview: https://1km-git-fork-urish-change-addr-button.1km.vercel.app

@guytepper guytepper merged commit 487b517 into guytepper:master Oct 5, 2020
@guytepper
Copy link
Owner

Thank you <3
It messed a bit with the list layout; opened a new issue over #33

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.

Add an option to change the current address
2 participants