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

[88] Map: style toggles #96

Merged
merged 26 commits into from
Oct 4, 2021
Merged

[88] Map: style toggles #96

merged 26 commits into from
Oct 4, 2021

Conversation

Nutto55
Copy link
Contributor

@Nutto55 Nutto55 commented Sep 30, 2021

✅ DoD

📝 Summary

  • Add 2 buttons to provide abilities to toggle map style between default(street) and satellite with show/hide map labels option each

💉 Testing

  • Select the data between April 1, 2021 - April 7, 2021.
  • Try toggle the pen and artist icons on the top right of map component and see the change

🛑 Problems

  • The current version have looping to setting label display on map style. If there is a performance issue, then we can ask to create a custom map to handle all the case.
  • I think the UI need some finetune which @naluinui can suggest a better idea.
  • Green color is look unclear when it is on satellite map

💡 More ideas

  • We should check if the user have performance issue before we asking to create a custom map to handle all the case.

📸 Screenshots

20210930_213701

🙋 Reviewer Guidance

  • Please check my looping logic and UI carefully.

@Nutto55 Nutto55 self-assigned this Sep 30, 2021
Copy link
Contributor

@charles-allen charles-allen left a comment

Choose a reason for hiding this comment

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

CHANGELOG.md Outdated
@@ -16,3 +16,4 @@
* **features:** Improve bar chart to support group bar chart (#58)
* **features:** Getting the comparison filter box ready for chart data (#59)
* **features:** Map of species count by site (#41)
* **features:** Map style toggle (#88)
Copy link
Contributor

Choose a reason for hiding this comment

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

Replies to your PR...

The current version have looping to setting label display on map style. If there is a performance issue, then we can ask to create a custom map to handle all the case.
...
We should check if the user have performance issue before we asking to create a custom map to handle all the case.

Do you mean, there are 2 approaches to this?

  • 4x maps
  • 2x maps + loop to toggle layer visibility

I see no performance issues with your layer code, and I think you picked the more intuitive approach! Since I think the performance depends on the map, not the datasets, I also expect this to be fine for other users & scenarios.

💚 TL;DR: I think you did a good job with the labels!

I think the UI need some finetune...
Green color is look unclear when it is on satellite map

IMO, this isn't a priority for your PBI/PR!!

CHANGELOG.md Outdated Show resolved Hide resolved
@charles-allen charles-allen changed the title Feature/88 map style toggle [88] Map: style toggles Oct 2, 2021
Copy link
Contributor

@naluinui naluinui left a comment

Choose a reason for hiding this comment

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

Chaz's comments already covered everything. One point I wanted to add is only the boolean variable name as mentioned in the comments below.

How about this for style:

ref:

Screen Shot 2564-10-02 at 08 36 45

Screen Shot 2564-10-02 at 08 37 03

Again you can create a new PBI to work on the style if needed

src/components/map-bubble/map-bubble.ts Outdated Show resolved Hide resolved
@naluinui
Copy link
Contributor

naluinui commented Oct 2, 2021

Here is how they handle it in Arbimon

Screen.Recording.2564-10-02.at.14.32.43.mov

@naluinui
Copy link
Contributor

naluinui commented Oct 4, 2021

This is the final design I proposed in one of Slack thread

IMG_7882

Toggle group button

  • Satellite (selected by default)
  • Streets

Check box button

  • ✔️ labels (checked by default)

@Nutto55
Copy link
Contributor Author

Nutto55 commented Oct 4, 2021

This is the updated UI change:
image

src/components/map-bubble/map-bubble.ts Outdated Show resolved Hide resolved
src/components/map-bubble/map-bubble.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@naluinui naluinui left a comment

Choose a reason for hiding this comment

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

only tiny UI that I've mentioned in Slack thread about moving label button up to the baseline

Screen Shot 2564-10-04 at 14 53 40

Apart from that, it's all nice work! Thanks Nutto!

@Nutto55 Nutto55 merged commit 90f8a0b into develop Oct 4, 2021
@Nutto55 Nutto55 deleted the feature/88-map-style-toggle branch October 4, 2021 15:27
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.

Map: style toggle
3 participants