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

Feature/custom range map #118

Merged
merged 178 commits into from
Jan 3, 2024
Merged

Conversation

maruk0chan
Copy link
Contributor

@maruk0chan maruk0chan commented Dec 26, 2023

Demo: https://hk-independent-bus-eta.web.app/

Screenshot 20231226 123326

Existing bug:
If mobile is turned landscape when the dialog is opened, the map has no space to show and the center will be off.
(possibly need to use css to assign element style when the mobile is in landscape, related to this PR)

- provide 4 range options 100m, 200m, 500m, 1km to users
accept searchRange prop in getSelectedRoutes and set 1000 as default
this is planned to be used in enable user setting search range
Copy link
Contributor

@chakflying chakflying left a comment

Choose a reason for hiding this comment

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

IMO it looks better with a bit more margin in front of "米".

image

Also, I tested this on a Pixel 4a, and dragging the slider is still painfully slow. Did a bit of profiling but looks like React is not the culprit (only taking 2ms per frame), most likely leaflet just can't update that fast.

I know it would be a bit more complicated, but only update the radius of the Circle after a bit of delay (or after mouse/finger release) would greatly improve performance.

Otherwise it seems to work okay 👍

@@ -422,3 +692,41 @@ const getSelectedRoutes = ({
),
};
};

const ListItem = styled("li")(({ theme }) => ({
margin: theme.spacing(0.5),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
margin: theme.spacing(0.5),
margin: `${theme.spacing(0.5)} ${theme.spacing(1)}`,

@chunlaw chunlaw merged commit 4bb4285 into hkbus:master Jan 3, 2024
@chunlaw
Copy link
Member

chunlaw commented Jan 4, 2024

Frankly speaking, your PR has merged many commits other than the feature of "custom range map". It is highly not recommended as other PRs are not all accepted, and this PR has even removed the .github/workflows/deploy.yml which is harming the routine update of the app.

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