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

Fix lines are again redrawn #798

Merged
merged 3 commits into from
Sep 18, 2019
Merged

Fix lines are again redrawn #798

merged 3 commits into from
Sep 18, 2019

Conversation

nikolai-b
Copy link
Contributor

Sadly this re-introduce double sorting when changing a region.
Fixes #797

Sadly this re-introduce double sorting when changing a region.
Fixes #797
@Robinlovelace
Copy link
Member

Confirmed issue demonstrated below on master.

Peek 2019-09-15 13-20

@Robinlovelace
Copy link
Member

Fix demonstrated after merging #798

Peek 2019-09-15 13-22

Copy link
Member

@Robinlovelace Robinlovelace left a comment

Choose a reason for hiding this comment

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

Tested with straight lines, cannot see any unintended consequences. Definitely fixes #797.

@Robinlovelace
Copy link
Member

Quick check-in question: can you think of any unintended consequences of this @nikolai-b or @usr110 ? I've had a look over the code and quick test and it seems fine but thought it worth checking. Thanks!

Changing the reactive $plot variable is triggering dynamic $plot getters to be reloaded:
this means that line sorting is causing the zones to be re-plotted
@nikolai-b
Copy link
Contributor Author

@Robinlovelace thank you for the quick test but I just wasn't happy that we were re-sorting the lines twice when changing region. I've fixed this in 440fe02 and noticed that we were also re-drawing zones af9c840
If your still happy I'll merge.

@Robinlovelace
Copy link
Member

Robinlovelace commented Sep 16, 2019

I think this is a definite improvement although don't have a very strong understanding of the this part of the code.

@nikolai-b when you say we're re-drawing the zones, could that also be slowing down the initial load? If so I'd suggest merging this into master and production and then working on that in a separate issue to keep things moving.

@usr110
Copy link
Member

usr110 commented Sep 16, 2019

Great work @nikolai-b

Just testing it and found an interesting phenomena. For london, specifically, if you move about the map - while the top lines remain as they are, they are redrawn. A lot of changes have been recently made, so I am not sure if it's a known issue or not.

Edit:
Just for comparison, I didn't find the same issue on the live-server.

Noticed that the production branch too got recently updated so can't really use it for comparison, but if my memory serves me right, this didn't use to happen.

@usr110
Copy link
Member

usr110 commented Sep 16, 2019

The lines are not redrawn - sorry about it @nikolai-b

Two things confused me:

  1. The plotting message saying that they are redrawn. I know they're used for local testing, but with messages like Plotting top 30 straight_lines took for straight_lines is slightly confusing.
  2. The loading circle - which is a minor issue.

thanks!

@nikolai-b nikolai-b merged commit da04a1f into master Sep 18, 2019
@nikolai-b nikolai-b deleted the line-redraw branch September 18, 2019 22:06
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.

Lines do not change when moving map
3 participants