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

Feature/add supercluster visualization #162

Merged
merged 27 commits into from
Oct 20, 2020

Conversation

ebefarooqui
Copy link
Contributor

@ebefarooqui ebefarooqui commented Oct 6, 2020

This is a preliminary PR to get feedback for the supercluster visualization. Currently, the styles are still in question (opacity, size of cluster, cluster radius) and we will be reviewing different styles soon to gauge what is best for the user.

See comment below for more updated information

@breezykermo
Copy link
Member

Love this feature. Do you want general comments regarding what's missing and bugs, or just inline comments on the code reviewing what you've done so far?

Copy link
Member

@breezykermo breezykermo left a comment

Choose a reason for hiding this comment

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

Really excellent work/feature! I've made a couple of structural suggestions around code quality. I haven't been able to test the feature yet in a browser, due to having the wrong config I think (have sent you a message privately).

src/common/utilities.js Show resolved Hide resolved
src/components/Map.jsx Outdated Show resolved Hide resolved
src/components/Map.jsx Outdated Show resolved Hide resolved
src/components/Map.jsx Outdated Show resolved Hide resolved
src/components/Map.jsx Show resolved Hide resolved
src/components/Map.jsx Show resolved Hide resolved
src/components/Map.jsx Outdated Show resolved Hide resolved
src/components/Map.jsx Show resolved Hide resolved
src/components/presentational/Map/Clusters.jsx Outdated Show resolved Hide resolved
Comment on lines +74 to +78
const extraRender = () => (
<React.Fragment>
{customStyles[1]}
</React.Fragment>
)
Copy link
Member

Choose a reason for hiding this comment

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

This extraRender logic is a good candidate for making into a HOC now, I think, given that it's used both here and in Events.jsx.

@ebefarooqui
Copy link
Contributor Author

This PR should be fully ready to review. I've attached the appropriate timemap config file here.

Note that the categories are currently broken on this branch, but you shouldn't need to test with them anyways. The styling for the clusters (opacity, size, cluster radius) are the ones decided on in the chat, but any and all suggestions welcome.

In order to run locally with updated data, I created a test_export_events tab in the datasheet, so in order to see the data locally, make sure that the return statement in src/lib.js in ds-s looks like the following:

return {
[test_export_events]: BP.deeprows,
[${prf('categories')}export_categories]: [BP.groups, BP.rows],
[${prf('associations')}export_associations]: BP.deeprows,
[${prf('sources')}export_sources]: BP.deepids,
[${prf('sites')}export_sites]: BP.rows
}

@ebefarooqui ebefarooqui merged commit 9b27d23 into develop Oct 20, 2020
@breezykermo breezykermo deleted the feature/add-supercluster-visualization branch October 21, 2020 10:53
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