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 region zooming when filtered by region. #226

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

itpcc
Copy link

@itpcc itpcc commented Mar 25, 2019

According to #222, I try to solve this issue by zooming to the selected region.

In this PR, when user use to filter by region,

  • ElectionMapContainer component will check whether the zoom attribute exists in the filter or not. If it exists, it'll pass to ElectionMap via component data name regionZoom.

  • In render of ElectionMap,

    • if regionZoom object is not empty, the map will be zoomed using D3Zoom's scaleTo and translateTo method.
    • else, it'll call resetZoom to clear the map to inital zoom state.

You can see the example in the screenshot below:

image

I develop and test using Google Chrome Desktop with the screen resolution at WXGA (1440x900). So, I'm not sure will it has any impact with the mobile-based browser?

If you have any suggestion, please reply to the message. I'm glad to help and make it better.

Regards,

@p16i
Copy link
Contributor

p16i commented Mar 25, 2019

Thanks!

@rapee @kristw could you please review this PR?

@dodadoa
Copy link
Contributor

dodadoa commented Mar 25, 2019

@itpcc have some problem on mobile, but I saw this on chrome simulator (Iphone X). could you please check with the real mobile?

image

@p16i
Copy link
Contributor

p16i commented Mar 25, 2019

Also not working properly on an iPad simulator
image

@itpcc
Copy link
Author

itpcc commented Mar 25, 2019

Maybe I should multiply with the screen ratio instead.

I'll do what I can.

@itpcc
Copy link
Author

itpcc commented Mar 25, 2019

As far as I can investigate, when using a mobile phone, if the tab 'map' is not active on page loading, the translateTo method seem to take the incorrect result. And, if I call the method manually, the position will be corrected.

But if the tab 'map' is already active on page load, the problem won't happen.

So, I'll detect tab changing event to re-position the map to solve this problem.

However, this needed to take more time than I expected because the event handling is so complex for me. Your suggestion will be very helpful.

Thanks in advance.

@codeclimate
Copy link

codeclimate bot commented Mar 25, 2019

Code Climate has analyzed commit 9152c89 and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 3

The test coverage on the diff in this pull request is 0.0% (0% is the threshold).

This pull request will bring the total coverage in the repository to 8.0% (0.0% change).

View more on Code Climate.

@itpcc
Copy link
Author

itpcc commented Mar 25, 2019

Okay, I've found currentMobileTab in ZoneMasterView is control how to display mobile's tab.
I solve the problem by passing that props to ElectionMap data, So, when it changes, the map will re-render and correct position.

@heytitle , @dodadoa , please check the result again.

Thanks

@kristw
Copy link
Collaborator

kristw commented Mar 25, 2019

Umm, thank you for your contribution krub. However, this approach is not robust a krub since all translation and scale are hard coded, which is subjected to size, padding and especially screen size. Otherwise you will have to hard code all combination of transform per major screen size.

I was working on a method that computes bounds from all the zones in each region and compute that back to transform, which will work regardless of the changes to those aforementioned parameters.

I would put this one on hold kon na krub. Please do not merge yet.

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.

4 participants