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

Refactor updating scorecard #169

Merged
merged 4 commits into from
Jan 15, 2024

Conversation

tunglinn
Copy link
Collaborator

Why this refactor?
This simplifies when scorecard updates.

Before, there were many different cases in which a scorecard would update, like the dropdown changing, the user panning to a different city, or the user clicking on a city.
Now, the scorecard only updates to the city closes to the center of the screen. So, when the dropdown changes, the map snaps to city, which will also automatically update the scorecard.

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Nice! Although technically this isn't a refactor because there is a behavior change for end users. Refactors are when the end behavior is the same and you only change how the code is implemented.

Comment on lines 168 to 169
const snapToCity = async (map, cityProperties) => {
const { layer } = cityProperties;
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to directly take the layer as the argument since it's the only part of cityProperties that you use. Callers would pass cityProperties.layer as the argument.

@tunglinn
Copy link
Collaborator Author

Nice! Although technically this isn't a refactor because there is a behavior change for end users. Refactors are when the end behavior is the same and you only change how the code is implemented.

There shouldn't bee a behavior change for the end user. Is there one that you noticed?

@Eric-Arellano
Copy link
Contributor

Didn't it change like this?

Before, there were many different cases in which a scorecard would update, like the dropdown changing, the user panning to a different city, or the user clicking on a city.
Now, the scorecard only updates to the city closes to the center of the screen. So, when the dropdown changes, the map snaps to city, which will also automatically update the scorecard.

My understanding of the code is that you now call snapToCity fewer times than before. But I didn't check this very rigorously.

* Centers view to city.
*
* @param map: The Leaflet map instance.
* @param cityProperties: An object with a `layout` key (Leaflet value) and keys
Copy link
Contributor

Choose a reason for hiding this comment

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

Outdated

@tunglinn
Copy link
Collaborator Author

Didn't it change like this?

Before, there were many different cases in which a scorecard would update, like the dropdown changing, the user panning to a different city, or the user clicking on a city.
Now, the scorecard only updates to the city closes to the center of the screen. So, when the dropdown changes, the map snaps to city, which will also automatically update the scorecard.

My understanding of the code is that you now call snapToCity fewer times than before. But I didn't check this very rigorously.

@tunglinn tunglinn closed this Jan 10, 2024
@tunglinn tunglinn reopened this Jan 10, 2024
@tunglinn
Copy link
Collaborator Author

Yes, but the end user experience doesn't/shouldn't change.

@tunglinn tunglinn merged commit f8f9d6f into ParkingReformNetwork:main Jan 15, 2024
1 check passed
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