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

GeoIP Map #142

Merged
merged 9 commits into from Jun 13, 2021
Merged

GeoIP Map #142

merged 9 commits into from Jun 13, 2021

Conversation

CasperVerswijvelt
Copy link
Contributor

@CasperVerswijvelt CasperVerswijvelt commented May 22, 2021

Unchanged from my last post in #92

Only added small tweak to remove the limited-height on the session list where the content is already limited to 10 items, causing annoying scroll on there

Screenshots:

Phone Desktop
analytics casperverswijvelt be_dashboard_service_ba385d2a-5c80-4b31-931e-3da309fac8d0__startDate=2021-4-22 endDate=2021-4-24(iPhone X) image

Closes #92

@CasperVerswijvelt CasperVerswijvelt changed the title Map chart GeoIP Map May 22, 2021
@milesmcc
Copy link
Owner

Sweet. I made a few changes, but nothing that drastic — namely, I adjusted the colors to match the rest of the dashboard, and moved this interface into the smaller country table. Ready for the next release!

@milesmcc milesmcc merged commit 9832de0 into milesmcc:master Jun 13, 2021
@CasperVerswijvelt
Copy link
Contributor Author

@milesmcc I'm not sure if removing the table version of the country data was the right move. Although the Geo map looks pretty cool, it doesn't show the same kind of overview like the table does and I do miss that a bit. Also on mobile the table is alot more useful. Maybe we can bring back the table by being able to toggle between map/table view?

Something else I noticed is that in the released version, the map does not extend to the edge of the card anymore (see screenshot) , which it did in my version. Is this intentional?

Screenshot_20210614-232232835~2

@milesmcc
Copy link
Owner

I knew someone would suggest bringing the country list back 🙂. Having both would be redundant, but feel free to submit a PR that adds a toggle.

I thought about this but didn’t implement it because we don’t yet have a good way to support client-side interactivity, and manually hiding/showing two different divs using JS felt janky. Perhaps we use StimulusJS? I’m not sure.

As for adding padding to the sides, that was intentional. We maintain padding everywhere else, so it looked off to me with it removed.

@CasperVerswijvelt
Copy link
Contributor Author

Why do you think using vanilla js would be janky? We could just use some inline js on the toggle button and then add a couple css rules, like this:

HTML
onclick="document.getElementById('geo-card').classList.toggle('geo-card--use-table-view')"

CSS

.geo-table {
  display: none;
} 

.geo-card--use-table-view .geo-map {
  display: none;
} 

.geo-card--use-table-view .geo-table {
  display: inline-block
} 

I don't really think adding a js framework for this is needed, but that might be something to look at for the long term

@haaavk
Copy link
Contributor

haaavk commented Jun 15, 2021

Vanilla js is the best for small amount of code. This project may never need a full js framework.
There are some interesting micro frameworks like Stimulus or Alpine.js.
I personally don't trust young js frameworks because they come and go too often.

@CasperVerswijvelt
Copy link
Contributor Author

Got it working like this now:

Table view Map view
image image

Not really happy about the toggle button in table view but can't think of anything else atm.

Code is on my geo-table-map branch

@milesmcc
Copy link
Owner

@CasperVerswijvelt this looks like a good start! What if we instead put the map and table buttons left aligned (i.e., after Country and Sessions by Geography) and renamed them to "(Switch to Table View)" and "(Switch to Map View") or something like that?

@CasperVerswijvelt
Copy link
Contributor Author

Yea that does make more sense visually 😛 I'll try that out tomorrow

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.

GeoIP Map
3 participants