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

Added buttons below map #40

Merged
merged 5 commits into from
May 6, 2023
Merged

Added buttons below map #40

merged 5 commits into from
May 6, 2023

Conversation

sodamodo
Copy link
Collaborator

@sodamodo sodamodo commented May 6, 2023

Overview

Give a brief description of the changes

Closes ###
Issue #28

Demo

image

Testing instructions

  • Add a list of instructions to show your changes working
  • Bullet points are desired
  • Checkboxes help for ensuring a specific quality or number

Checklist before merge

  • Update the CHANGELOG.md
  • Rebase your changes from whatever branch you're merging to

@sodamodo sodamodo requested a review from travissouthard May 6, 2023 17:22
@travissouthard travissouthard self-assigned this May 6, 2023
Copy link
Collaborator

@travissouthard travissouthard left a comment

Choose a reason for hiding this comment

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

Thanks for putting this up. There's a big styling note below with an example. We want to ensure the buttons always appear on screen without having to scroll down.

CHANGELOG.md Outdated
@@ -13,6 +13,7 @@ and this project adheres to release number versioning.
- Added Docker containers with docker-compose [#10](https://github.com/CodeForPhilly/third-places/pull/10)
- Set up Leaflet React component and boiletplate homepage[#7](https://github.com/CodeForPhilly/third-places/issues/7)
- Added initial Django models & migration [#21](https://github.com/CodeForPhilly/third-places/pull/21)
- Added buttons at bottom of Map [#28](https://github.com/CodeForPhilly/third-places/issues/28)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will actually be for #40, the link should point to the PR rather than the issue

Comment on lines 4 to 9
}

.button-container {
padding-left: 1rem;
padding-right: 1rem;
margin-top: 2rem;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I played around with the styles locally and to get a style that didn't push the buttons down to a place the user couldn't scroll to since the map would just move, I got this to render like the wireframes:

.leaflet-container-wrapper {
  display: flex;
  flex-flow: column nowrap;
  height: 100vh;
}

.leaflet-container {
  width: 100vw;
  height: 100%;
  flex-grow: 1;
}

.button-container {
  display: flex;
  flex-flow: row nowrap;
  justify-content: center;
  z-index: 1000;
  width: 100vw;
  padding: 1rem 0;
  background-color: white;
}

Examples:
image
image
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

God bless you flexbox

</div>
<div className="button-container">
<Button style={{backgroundColor:"#ADD8E6", marginLeft: '1rem', marginRight: '1rem'}} variant="primary">Near Me</Button>
Copy link
Collaborator

Choose a reason for hiding this comment

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

The objects in the style section are all the same and could be refactored out to a const variable and used in each of the style props.

Copy link
Collaborator

@travissouthard travissouthard left a comment

Choose a reason for hiding this comment

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

Thanks for making that change so quickly!

@sodamodo sodamodo merged commit 66dd1ad into develop May 6, 2023
@themightychris themightychris mentioned this pull request May 10, 2023
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.

2 participants