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

Printable reports (and other miscellany) #56

Closed
wants to merge 17 commits into from

Conversation

mojodna
Copy link
Collaborator

@mojodna mojodna commented Dec 6, 2016

This implements the "report" page along with print CSS for it, re-using the existing MapView and FeatureModal components (with some necessary modifications).

When 62 or fewer ([A-z0-9]) features are displayed, their markers are labeled on the map and associated with the marker icon on their individual page.

Filters are also persisted in the URL and are retained when switching tabs.

This also throws in a .nvmrc (for node-6.x) and an ESLint config (I use it with Atom).

@gmaclennan
Copy link
Member

gmaclennan commented Dec 7, 2016

@mojodna this is looking great! thanks for the work on this. Some comments:

  • Bug: starting in Map view and switching to report view causes chrome to lock up, and eventually kill the page. If you start in Report view and switch to map view there is no problem. I don't think it is a rendering issue, since adding features.slice(0,10).map into the report view and the problem still persists.
  • Labelling: let's start with uppercase letters, then lowercase, then numbers - I think numbers can confuse with the suggestion that they mean something else.
  • Layout: Can we get a page layout view like https://invis.io/J63YJ6274#/82911239_Print_View ? I'm not sure we need those options at this time, but it would be good to show wysiwig page layout.
  • Layout: Map should fill the page.
  • Styling: Can we get labels in icons on feature pages in Roboto to match everything else.
  • Layout: Can we maybe compress line-spacing in the table view to fit more on each page? Not sure about the best way to deal with data crossing pages right now. I do want to have a configuration option to choose which fields and in which order are shown in the detail view, but I don't think that is a priority for this trip, rather a nice-to-have.
  • Map Style: I'll take the lead on this - I want to experiment with the labels on icons to make them look a bit better.
  • Map Style: For some reason some icons are not appearing on the map. If you look at the report for the filter "only fishing" points C, D and E have no icons: http://localhost:9966/report?filter=eyJoYXBwZW5pbmciOnsiaW4iOlsiZmlzaGluZyJdfX0

@mojodna
Copy link
Collaborator Author

mojodna commented Dec 7, 2016

For some reason some icons are not appearing on the map.

I was noticing that. Making them all the same color seems to resolve that (so it may be the result of "missing" markers, which could be causing other data to be missing elsewhere).

Thoughts on how to force them to all be the same color / why it's not filtering on happening (which should achieve the same thing)?

https://github.com/digidem/mapfilter/pull/56/files#diff-83418c5c1bc6a57e2dad4fbf90122b1fR69

@mojodna
Copy link
Collaborator Author

mojodna commented Dec 7, 2016

starting in Map view and switching to report view causes chrome to lock up

It's not the map, fortunately. Something is triggering aggressive re-rendering that I need to track down.

})
}

const geojson = getMapGeoJSON(assign({}, state, {
Copy link
Member

Choose a reason for hiding this comment

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

This will be causing re-rendering. getMapGeoJSON is a memoized function. but the argument is different on every call, so geojson will be different, so redux, which does a shallow compare on shallow compare to stop re-rendering, will continually rerender.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's what I suspected; I'm refactoring that now.

Copy link
Member

Choose a reason for hiding this comment

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

The most efficient approach I think is to pass the map the entire geojson file along with the filter, and let Mapbox-gl-js handle the filtering internally, and then for the features have that separate, pre-filtered.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Pushing the filtering back into the MapView fixed the memoization and coloring, but at the expense of losing the labels (since they're no longer the first n features)...

@gmaclennan
Copy link
Member

I’ll have a look at the icons / coloring a bit later this afternoon (have some calls first). The coloring was always a little fragile. Mapbox-gl-js wants icons as PNGs, and rendering them as SVG would be a performance hit. We're waiting on mapbox/mapbox-gl-js#2059 to land to be able to dynamically add icons and not have a dependency on a specific mapbox style that needs to be kept in sync with the color list. Anyway, I'll investigate because I remember some of the hackiness to make this work.

@mojodna
Copy link
Collaborator Author

mojodna commented Dec 7, 2016

I'm spinning my wheels pretty hard on reintroducing labeling. The fundamental problem is that MapView expects geojson (which is unfiltered, augmented with __mf_id and __mf_color) and the list of FeatureModals expects individual features. Since I only want to display filtered features, I'm working from the features array (which is filtered, but not augmented).

Passing features (after being munged into a GeoJSON FeatureCollection) that have had labels applied to them into the MapView works, but the coloring disappears. This is what was happening prior to fcd64e2, but that had memoization issues and labels that should have all been the same color weren't.

At one point I was adding labels to both features and geojson (split by colored field), but the ordering ends up varying (it shouldn't, but it seems to) between the 2 lists.

@okdistribute
Copy link
Contributor

This seems to be stale!

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.

3 participants