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

feat: update design of zoom controls on new map #3880

Merged
merged 2 commits into from
Oct 8, 2024

Conversation

JoseAConcepcion
Copy link
Contributor

@JoseAConcepcion JoseAConcepcion commented Oct 1, 2024

PR Checklist

PR Type

What kind of change does this PR introduce?

  • Bugfix (fixes an issue)
  • Feature (adds functionality)
  • Code style update
  • Refactoring (no functional changes)
  • Build related changes
  • CI related changes
  • Documentation changes
  • Other... Please describe:

What is the current behavior?

The zoom controls for the new map are currently the default design.

What is the new behavior?

The zoom controls were updated with the correct design.
old_design
new_design

Does this PR introduce a breaking change?

  • Yes
  • No

Git Issues

Closes #3872

What happens next?

Thanks for the contribution! We try to make sure all PRs are reviewed ahead of our monthly maintainers call (first Monday of the month)

If the PR is working as intended it'll be merged and included in the next platform release, if not changes will be requested and re-reviewed once updated.

If you need more immediate feedback you can try reaching out on Discord in the Community Platform development channel.

@JoseAConcepcion JoseAConcepcion requested a review from a team as a code owner October 1, 2024 18:45
Copy link

cypress bot commented Oct 1, 2024

onearmy-community-platform    Run #6338

Run Properties:  status check passed Passed #6338  •  git commit 4e1be1ea00: fix: padding and size of the zoom buttons
Project onearmy-community-platform
Run status status check passed Passed #6338
Run duration 04m 44s
Commit git commit 4e1be1ea00: fix: padding and size of the zoom buttons
Committer Jose
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 75

Copy link
Member

@benfurber benfurber left a comment

Choose a reason for hiding this comment

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

Fantastic, thank you @JoseAConcepcion!

Minor feedback.

  1. Make the size of the buttons even bigger. Let's go with 22px font size.
  2. More spacing in the element - i.e. around the text.
  3. More spacing around - around the element should be the same size spacing other components on the page have (ideally added to the parent element so it can be added through style props rather than css).

@JoseAConcepcion
Copy link
Contributor Author

Hi @benfurber, the font size of the buttons already were 25px. Should it be higher still? Did you mean the general size of the actual buttons?

@benfurber
Copy link
Member

Hi @benfurber, the font size of the buttons already were 25px. Should it be higher still? Did you mean the general size of the actual buttons?

@JoseAConcepcion Ha. I misread the code! Yes I mean the general size of the buttons. If it's 25px already, how's about 30px?

@JoseAConcepcion
Copy link
Contributor Author

JoseAConcepcion commented Oct 3, 2024

@benfurber its bigger now, increased the size and the padding of the element but I'm not sure it's the spacing of the other elements, for some reason modifying the parent doesn't do the trick :(

feedback_design

PD: While I was zooming in and out discovered that "the report a problem" button disappears, not sure to open an Issue because there are other user buttons to be added in the new design.

@benfurber
Copy link
Member

Awesome! Thanks @JoseAConcepcion, I'll get this merged in now. Anything else on the issues list take your fancy?

@benfurber
Copy link
Member

@all-contributors add @JoseAConcepcion for code

Copy link
Contributor

@benfurber

I've put up a pull request to add @JoseAConcepcion! 🎉

@benfurber benfurber merged commit 890c92e into ONEARMY:master Oct 8, 2024
19 of 20 checks passed
@JoseAConcepcion
Copy link
Contributor Author

@benfurber happy to help :)
I will be back in two weeks give or take, got to finish some final projects and exams.
For sure I will take another issue.

@onearmy-bot
Copy link
Collaborator

🎉 This PR is included in version 2.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

feat: update design of zoom controls on new map
3 participants