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 map legend #115

Merged
merged 10 commits into from Jun 18, 2020
Merged

Add map legend #115

merged 10 commits into from Jun 18, 2020

Conversation

anaPerezGhiglia
Copy link
Contributor

@anaPerezGhiglia anaPerezGhiglia commented Jun 18, 2020

Map legend 🗺️

  • All the data is now being clustered in 5 groups
  • values are rounded to the three-most-significant digit
  • values are only rounded in the legend
    Rounded breaks are not used for clustering the data, since it can happen that the rounded upper limit < some regions cases. If this happens, it would end in those regions not being member of any cluster.
    For example, if a region has 2,163,543 active cases, the rounded limit would be 2,160,000. Thus, if clustering with such upper-limit would end with such region out of the cluster

UI tweaks 🔩

For showing the legend alongside all the data that was already being displayed without having overlapping, some rearrangements where needed:

  • New wording latest totals
  • New wording actives
  • New layout for showing latestUpdate
  • Smaller status dot size 🔴
  • Add toLocaleString() to numbers (UI tweaks  #100)
  • Rearrange of suggestions, latestUpdate and userStatus order

Mobile

Desktop

Bonus ➕

  • remove unnecessary docker-compose up db from dev-setup.sh

closes #94

pyritewolf
pyritewolf previously approved these changes Jun 18, 2020
Copy link
Contributor

@pyritewolf pyritewolf left a comment

Choose a reason for hiding this comment

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

As usual I left some suggestions & educational comments, but if you think they'll take too long to implement feel free to just merge!

@@ -64,16 +65,21 @@ def cluster_data(confirmed, clusters_config=None):
df["confirmed"], nb_class=clusters_config["clusters"]
)

rounded_breaks = list(map(lambda limit: round(limit, sigfigs=3), breaks))
Copy link
Contributor

Choose a reason for hiding this comment

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

100% open to discussion, but, do you really think it's worth it to add another lib to the backend when the front can do this out of the box?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I add it in the back since

  • at first was considering this rounded limits for clustering the data, then realized that it was not a good solution for what i mentioned in the PR description
  • I like the fact that front doesn't make decisions about the data itself, it just formats it in the way it likes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and I don't feel like rounding the data is just format ..

@@ -7,7 +7,6 @@ pre-commit install -f

# lift & update containers
docker-compose build
docker-compose up db
Copy link
Contributor

Choose a reason for hiding this comment

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

ty so much

clusters &&
clusters.map((range, i) => {
return {
label: `${range[0].toLocaleString()} - ${range[1].toLocaleString()}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

See, here's were we could be doing the native rounding I was speaking about before with either this native JS thing I mentioned before, or actually, passing some options like maximumSignificantDigits to the toLocaleString() we're already using!

Comment on lines 236 to 237
{legendRanges.map((range, i) => {
return (
Copy link
Contributor

Choose a reason for hiding this comment

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

JS Syntax Sugar Tip of the Day! This:

() => {
   return (whatever)
}

is the same as this:

() => (whatever)

border-radius: 3px;
bottom: 30px;
box-shadow: 0 1px 2px rgba(0, 0, 0, 0.1);
font: 12px/20px "Helvetica Neue", Arial, Helvetica, sans-serif;
Copy link
Contributor

Choose a reason for hiding this comment

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

There's way too many fonts in this project, and I'm pretty sure Helvetiva Neue isn't one of them. Can we use either Roboto or one of the other already existing fonts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, I stole this config from an example promising I would review then what was really necessary, and I completely forgot about it. Will change

}

.header h3{
margin-block-end: 0.5em;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest using margin-bottom and lowering it to 0.2rem. Always prefer rem to em!

em vary depending their location on the dom tree & surrounding rules. rem vary according to the font-size given to html, which gives us more control.

);

const latestUpdate = () => (
<div>
Copy link
Contributor

Choose a reason for hiding this comment

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

This div isn't really necessary, I'd change it for an empty JSX parent <>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't know the <>
It seemed to me unnecessary adding the div
Will change 🙂

);

const suggestions = () => (
<div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as before here: this div isn't really necessary, I'd change it for an empty JSX parent <>

const suggestions = () => (
<div>
<h3>SUGGESTIONS</h3>
<div>Stay at home</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

This div is only used to add a linebreak. Since we're tweaking this whole bit of code, I'd change it for a p: it's more semantic / describes better what the element is, and that's the whole point of HTML!

But bear in mind, p usually has a default margin / padding (I can never remember which!), so you'll probably have to clear that up in CSS with something that looks like

.root p { margin: 0; }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good

@anaPerezGhiglia anaPerezGhiglia merged commit 4c19868 into master Jun 18, 2020
@anaPerezGhiglia anaPerezGhiglia deleted the feature/94-map-legend branch June 18, 2020 21:58
@easpencer
Copy link

Not seeing the legend update when cloropeth level changes

@anaPerezGhiglia
Copy link
Contributor Author

@easpencer since #98 is not necessary since the data is clustered taking into account all the regions information - whether is country or state

@anaPerezGhiglia
Copy link
Contributor Author

@easpencer for future comments: is more easy to track all the comments and interactions in the issue #94 itself than in the PR, even more so when the PR is closed 🙂

@easpencer
Copy link

easpencer commented Jun 19, 2020 via email

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 map legend on <Dashboard />
3 participants