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

Draggable user marker #29

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

idoco
Copy link

@idoco idoco commented Oct 5, 2020

The browser location service is not very accurate on desktops, so I'd love it if the positionMarker will draggable (great for small adjustments)

Once #20 will be merged this should be a small change. I used @urish context.js to get the dispatch, but couldn't test it until it is merged.

Let me know if I should proceed with this.

@idoco
Copy link
Author

idoco commented Oct 10, 2020

Is this relevant? Should I resolve the conflicts or close it?

@guytepper
Copy link
Owner

guytepper commented Nov 7, 2020

Hi @idoco - I don't know how I missed this PR, but noticed it just now - sorry!
We do need a static marker on the initial user position. Making it follow the user center won't be a good UX in my opinion (look at any major map service, where the marker stays in it's initial position).

So, I'm closing the PR for now , but let me know if you have different thoughts regarding this!

@guytepper guytepper closed this Nov 7, 2020
@idoco
Copy link
Author

idoco commented Nov 7, 2020 via email

@guytepper
Copy link
Owner

I understand.
Now that you can change the address manually, I guess it's not relevant anymore.
Sorry again for missing this.

@idoco
Copy link
Author

idoco commented Nov 7, 2020 via email

@guytepper
Copy link
Owner

What's the use case for this? If a desktop visitor tries to get their GPS position, isn't it way off anyway?

@idoco
Copy link
Author

idoco commented Nov 7, 2020 via email

@guytepper
Copy link
Owner

Okay, I understand the use case now, so I'm reopening the PR - would you like to keep work on it, updating it to reflect the changes made since?

I think the only change it needs is to send the coordinates to MobX's setCoordinates action on the RootStore, instead of passing it to the reducer (which we don't use anymore).

@guytepper guytepper reopened this Nov 7, 2020
@idoco
Copy link
Author

idoco commented Nov 7, 2020

Thanks Guy! Yes, I’ll make the changes and submit it again for review

@guytepper
Copy link
Owner

Hi @idoco, were you able to progress with this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants