Add globe option to map view #3057
Conversation
| strokeWidth={ | ||
| (isFocus ? focusStrokeWidth : 0.3) / | ||
| viewportScale | ||
| } | ||
| stroke={stroke} | ||
| strokeOpacity={strokeOpacity} | ||
| cursor="pointer" | ||
| fill={`url(#${Patterns.noDataPatternForMapChart}-${this.manager.projection})`} | ||
| // Hashing looks weird as it doesn't rotate with the rest of the globe |
There was a problem hiding this comment.
I think this looks clearer than the hashing lines, but could see the arguments for both ways.
There was a problem hiding this comment.
I think the hashing lines are the right approach here. Any solid colour (including the gray that you have chosen) could be used to indicate a data value, in which case countries with that data value and countries without data would look the same.
|
Hey, @JohnGinger, this looks great! Thank you so much for your contribution! We will have an internal discussion about whether adding a Globe view fits into the roadmap for Grapher. If we do want to go ahead with this change, would you be open to working some more on this PR to tweak it according to our needs? |
|
Thanks @sophiamersmann - yes happy to tweak it if you tell me what you need tweaking. |
|
Hey @JohnGinger Great! Thanks again for your work so far. A globe view would be a nice addition to Grapher, and we’re excited about adding it! There are a couple of things we would like to see implemented before shipping:
I noticed a couple of bugs when interacting with the globe that deserve to be looked at:
Please let me know if you have any questions or if there is anything I can help you with. Happy globe-ing! Globe positioning video: globe.mov |
|
Sorry for the delay, here are the requested changes, I think I've resolved all those issues. Screen.Recording.2024-01-15.at.16.46.04.movI've removed the spinning as I was concerned about getting the performance correct when using mobX (I'm sure there is an easy idiomatic way - but I couldn't work out what it was). Happy to add it back if you can give me some pointers on animation in the rest of the code. Unfortunately I wasn't able to work out a way to test this on mobile. |
|
Hey @JohnGinger, that looks great! I'll have a closer look at the end of the week :) |
There was a problem hiding this comment.
Hey @JohnGinger
I finally had some time to look into this today. Most of my comments relate to MobX and how to make the dragging action more performant. I suggest using requestAnimationFrame to re-draw the globe only when necessary. requestAnimationFrame should also help when building a spinning animation.
Let me know if you have any questions!
| @@ -309,6 +313,10 @@ export class MapChart | |||
| this.onLegendMouseLeave() | |||
| } | |||
|
|
|||
| @observable isGlobe(): boolean { | |||
There was a problem hiding this comment.
isGlobe can be inferred from state, and thus should be a computed property:
@computed get isGlobe(): boolean {
return this.manager.isGlobe ?? false
}| } | ||
|
|
||
| getPath(feature: RenderFeature): string { | ||
| if (!this.globeProjection) { |
There was a problem hiding this comment.
globeProjection should also be a computed property. If it is, you don't need this extra if clause.
| if (this.manager.isGlobe()) { | ||
| if (this.manager.projection !== MapProjectionName.World) { | ||
| this.globeProjection?.scale(this.globeScale * 1.8) | ||
| this.globeProjection?.center([0, 0]) | ||
| this.globeProjection?.rotate(this.viewport.rotation) | ||
| } else { | ||
| this.globeProjection?.scale(this.globeScale) | ||
|
|
||
| this.globeProjection?.translate([ | ||
| this.bounds.width / 2, | ||
| this.bounds.height / 2, | ||
| ]) | ||
|
|
||
| this.globeProjection?.center([0, 0]) | ||
| this.globeProjection?.rotate(this.viewport.rotation) | ||
| this.globeProjection?.rotate(this?.dragging) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
If you define globeProjection as a computed property, then you don't need this code.
| const sensitivity = 1.25 | ||
| const rotateX = rotate[0] + ev.movementX * sensitivity | ||
| let rotateY = rotate[1] - ev.movementY * sensitivity | ||
| rotateY = Math.max(-90, Math.min(90, rotateY)) |
There was a problem hiding this comment.
I don't understand why rotateY gets special treatment here?
There was a problem hiding this comment.
The maths of rotating gets confusing if you go all the way round the globe (and everything goes upside down). I thought it was easier and also a better user experience to keep things the 'right' way up.
| if (this.enableDragging) { | ||
| const rotate = this.globeProjection.rotate() | ||
| const sensitivity = 1.25 | ||
| const rotateX = rotate[0] + ev.movementX * sensitivity |
There was a problem hiding this comment.
Apparently, browser implementations for movementX/Y are inconsistent. I think we're better off computing our own deltas based on screenX/Y.
| let rotateY = rotate[1] - ev.movementY * sensitivity | ||
| rotateY = Math.max(-90, Math.min(90, rotateY)) | ||
|
|
||
| this.dragging = [rotateX, rotateY] |
There was a problem hiding this comment.
This triggers an (expensive) re-render. We only want to do this when necessary. Wrapping this code in requestAnimationFrame should do the trick.
| ev.preventDefault() /* Without this, title may get selected while shift clicking */ | ||
| }} | ||
| onMouseMove={(ev: React.MouseEvent<SVGGElement>) => { | ||
| if (this.enableDragging) { |
There was a problem hiding this comment.
Can you move this whole block into the onMouseMove function? Mobx state changes should be run in actions. If you check out the function definition of onMouseMove, you'll see that it's decorated with @action.bound which is important here.
| @@ -638,6 +646,9 @@ class ChoroplethMap extends React.Component<{ | |||
| manager: ChoroplethMapManager | |||
| }> { | |||
| base: React.RefObject<SVGGElement> = React.createRef() | |||
| globeProjection: any | |||
| dragging: any | |||
There was a problem hiding this comment.
This should be an observable, i.e. @observable dragging, since updating dragging should trigger a re-render.
Nit: globeRotation or something similar might be a bit more fitting as a name.
|
Thanks for your detailed comments @sophiamersmann - will have a proper look at them early next week. |
|
Great! Thank you :) I almost forgot... our designer came back with a sketch. The Globe view switcher should sit next to the Continent switcher in the control bar. It should be fairly easy for you to add. We have a LabeledSwitch component you can use, and the Continent switcher is rendered here.
|
|
Warning Rate Limit Exceeded@JohnGinger has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 42 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe Our World in Data Grapher repository has been updated to include globe visualization capabilities. This enhancement involves adding a Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Screen.Recording.2024-01-28.at.23.01.21.movMade those markups - sorry for the delay - thanks for the tips @sophiamersmann - performance is much better now! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (6)
- packages/@ourworldindata/grapher/src/captionedChart/CaptionedChart.tsx (5 hunks)
- packages/@ourworldindata/grapher/src/chart/ChartManager.ts (1 hunks)
- packages/@ourworldindata/grapher/src/core/Grapher.tsx (1 hunks)
- packages/@ourworldindata/grapher/src/mapCharts/MapChart.sample.ts (1 hunks)
- packages/@ourworldindata/grapher/src/mapCharts/MapChart.tsx (15 hunks)
- packages/@ourworldindata/grapher/src/mapCharts/MapChartConstants.ts (2 hunks)
Additional comments: 10
packages/@ourworldindata/grapher/src/mapCharts/MapChartConstants.ts (1)
- 58-58: The addition of the
isGlobeproperty toMapChartManageras an optional boolean is appropriate for enabling a toggle between map and globe views. This change aligns with the PR's objective to introduce a globe view option.packages/@ourworldindata/grapher/src/chart/ChartManager.ts (1)
- 36-36: Adding the
isGlobeproperty as an optional boolean to theChartManagerinterface is a good practice for extending functionality to support globe view across different chart types. This change is consistent and supports the feature's integration.packages/@ourworldindata/grapher/src/mapCharts/MapChart.sample.ts (2)
- 28-30: The update to the
dataproperty usingArray.fillandArray.mapmethods to generateyears,entities, andvaluesarrays is a clean and efficient way to create sample data for demonstration purposes. This approach is suitable for auto-generating data.- 38-107: The expansion of the
metadataproperty'sdimensions.entities.valuesarray to include additional country objects enhances the sample data's comprehensiveness. This change is beneficial for testing the globe view with a wider range of entities.packages/@ourworldindata/grapher/src/captionedChart/CaptionedChart.tsx (3)
- 27-27: The addition of
faEarthAmericasandfaMapicons from Font Awesome is directly related to the new globe view feature, indicating UI enhancements for toggling between map and globe views.- 71-71: The
isGlobeproperty added to theCaptionedChartManagerinterface as a boolean is crucial for managing the state of the globe view within the captioned chart component. This addition is necessary for the feature's functionality.- 296-304: The implementation of a
LabeledSwitchfor toggling the globe view within theCaptionedChartcomponent is a user-friendly way to switch between map and globe views. This UI component enhances the interactivity of the chart.packages/@ourworldindata/grapher/src/mapCharts/MapChart.tsx (2)
- 1-34: The reorganization of import statements improves readability and maintainability by grouping related imports together. This change aligns with best practices for code organization.
- 316-318: The addition of the
isGlobecomputed property is a straightforward and effective way to determine if the globe view is active based on the manager's state. This is a good use of computed properties for reactive UI updates.packages/@ourworldindata/grapher/src/core/Grapher.tsx (1)
- 374-374: The addition of the
isGlobeobservable property introduces a new feature toggle for enabling a globe view. Ensure that this property is utilized appropriately in the rendering logic and that any necessary UI components for toggling this feature are implemented and accessible to the user.
packages/@ourworldindata/grapher/src/mapCharts/MapChartConstants.ts
Outdated
Show resolved
Hide resolved
| projectionName: MapProjectionName, | ||
| isGlobe = false | ||
| ): RenderFeature[] => { | ||
| if (isGlobe) { | ||
| renderFeaturesCache.get(MapProjectionName.World) | ||
| } |
There was a problem hiding this comment.
The conditional check for isGlobe within renderFeaturesFor lacks an actual implementation. If isGlobe is true, it retrieves the world projection from the cache but does not proceed to use or return it, leading to potential unintended behavior or a no-op.
Consider implementing the intended behavior for when isGlobe is true or clarify the purpose of this conditional check.
|
|
||
| // Viewport for each projection, defined by center and width+height in fractional coordinates | ||
| @computed private get viewport(): { | ||
| rotation: any | ||
| x: number | ||
| y: number | ||
| width: number | ||
| height: number | ||
| } { | ||
| const viewports = { | ||
| World: { x: 0.565, y: 0.5, width: 1, height: 1 }, | ||
| Europe: { x: 0.53, y: 0.22, width: 0.2, height: 0.2 }, | ||
| Africa: { x: 0.49, y: 0.7, width: 0.21, height: 0.38 }, | ||
| NorthAmerica: { x: 0.49, y: 0.4, width: 0.19, height: 0.32 }, | ||
| SouthAmerica: { x: 0.52, y: 0.815, width: 0.1, height: 0.26 }, | ||
| Asia: { x: 0.75, y: 0.45, width: 0.3, height: 0.5 }, | ||
| Oceania: { x: 0.51, y: 0.75, width: 0.1, height: 0.2 }, | ||
| World: { | ||
| x: 0.565, | ||
| y: 0.5, | ||
| width: 1, | ||
| height: 1, | ||
| rotation: [30, 0], | ||
| }, | ||
| Europe: { | ||
| x: 0.53, | ||
| y: 0.22, | ||
| width: 0.2, | ||
| height: 0.2, | ||
| rotation: [-30, -50], | ||
| }, | ||
| Africa: { | ||
| x: 0.49, | ||
| y: 0.7, | ||
| width: 0.21, | ||
| height: 0.38, | ||
| rotation: [-30, 0], | ||
| }, | ||
| NorthAmerica: { | ||
| x: 0.49, | ||
| y: 0.4, | ||
| width: 0.19, | ||
| height: 0.32, | ||
| rotation: [70, -50], | ||
| }, | ||
| SouthAmerica: { | ||
| x: 0.52, | ||
| y: 0.815, | ||
| width: 0.1, | ||
| height: 0.26, | ||
| rotation: [70, 20], | ||
| }, | ||
| Asia: { | ||
| x: 0.75, | ||
| y: 0.45, | ||
| width: 0.3, | ||
| height: 0.5, | ||
| rotation: [-80, -20], | ||
| }, | ||
| Oceania: { | ||
| x: 0.51, | ||
| y: 0.75, | ||
| width: 0.1, | ||
| height: 0.2, | ||
| rotation: [-125, 30], | ||
| }, | ||
| } | ||
|
|
||
| return viewports[this.manager.projection] |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [650-821]
The implementation of globe dragging behavior, including the observable globeRotation property and modifications to onMouseMove, onMouseDown, and onMouseUp event handlers, introduces interactive rotation for the globe. However, there are a few considerations:
- Ensure
globeRotationis initialized properly to prevent undefined behavior during the first interaction. - The sensitivity and clamping of rotation angles in
onMouseMoveare hardcoded. Consider making these configurable through props or constants for easier adjustments. - The globe's projection and scale calculations in
globeProjectionare critical for performance and correctness. Verify that these calculations are optimized and accurately reflect the intended user experience.
Consider initializing globeRotation with a default value, making sensitivity and angle clamping configurable, and reviewing the projection and scale calculations for optimization and accuracy.
| getPath(feature: RenderFeature): string { | ||
| if (this.manager.isGlobe) { | ||
| return geoPath().projection(this.globeProjection)(feature.geo) ?? "" | ||
| } else { | ||
| return feature.path | ||
| } | ||
| } |
There was a problem hiding this comment.
The conditional logic in getPath to handle globe-specific path generation using geoPath().projection(this.globeProjection) is correctly implemented. This ensures that the correct path is generated based on whether the globe view is active. However, ensure that this method's performance is optimized, especially since it could be called frequently during rendering.
Review the performance of the getPath method, especially the creation of new geoPath instances, to ensure it is optimized for frequent calls.
…ts.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Great work! That's much better. We're almost there... The last missing feature to be implemented is automatic spinning of the globe. Do you want to have a go at that? I remember you saying that you ran into performance issues the first time you tried. If that's the case, try repeatedly calling const loop = () => {
// do something
// then request another frame
requestAnimationFrame(loop)
}
requestAnimationFrame(loop)Small note: I think it would be better to dismiss tooltips while dragging the globe. They're a little distracting. |
|
Hi, Happy to. Do you have a preference for where you want the spinning button to be? Next to the switcher? Best, |
|
The globe should automatically spin – there shouldn't be a button. |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
This PR has had no activity within the last two weeks. It is considered stale and will be closed in 3 days if no further activity is detected. |
|
Hey @JohnGinger, I hope you're good! I just wanted to check in with you about this PR. If you intend to do some more work on it, I can make the stale bot go away, and we can keep the PR open for longer. If you have moved on, that's okay, too. In that case, I would want to merge this PR into a feature branch on our repo so that one of us can pick up the work. Thank you, |
|
Hi, sorry for the delay - life has got a bit busy. I'm still happy to work on it - Is it just the rotation left? I have some time I can spend on this weekend. |
|
Great! Yes, the rotation is the last missing piece. There are some other details to be figured out (e.g. what the starting point of the rotation should be). Questions like these need some discussion, which always results in some back and forth, so it might be best for one of us to take over at that point. After you have implemented the rotation, I'd give it a review, and then we'd merge it into a feature branch so that one of us can figure out the last details. Does that sound good to you? |
|
Sounds good to me |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
@sophiamersmann - sorry for taking ages to get to this. I may still get to it, but I wouldn't be offended if you took it internally to finish it off. |
|
Hey @JohnGinger, no worries, thank you for letting me know! The best way forward is probably to merge this PR into a feature branch on |
|
Hey @JohnGinger, I now merged your changes into a feature branch (#3589). Thanks again for your great work! 🙌🏻 |
|
Thanks so much @sophiamersmann - sorry I've dropped this for ages |
I added a PR to add a globe tab in addition to the map view.
owid-map-globe.mov
I know this isn't really on a todo list, but I thought it would be quite a cool addition to have to the standard map projection, as they can be misleading. No worries if you don't think it adds much, or doesn't fit with the roadmap.
Ignore the data on the map, I just autogenerated something.
This is the first time committing to the repo, so let me know if there is anything I can do to clean up this PR
Summary by CodeRabbit