-
Notifications
You must be signed in to change notification settings - Fork 40
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
Find My Plane integration - revised #38
base: master
Are you sure you want to change the base?
Conversation
…orrectly displayed
…ltiple browsers open
Thanks for the great work! I'll try to test it next week so I can give you feedback. Unfortunately, I've been really busy the last couple of week so I didn't have much time for this app. |
Just checking in on this as to whether you have had a chance to review? |
@mracko Are you open to reconsidering this pull request to integrate Find My Plane? If so I will revise the code and open another pull request but don't want to if you're not open to it / this project is abandoned Thanks |
Thank you for your comments on the previous pull request.
Given there was a new master version I thought it easier to refork and do a fresh pull request, which this is. I will close the previous one and link here.
With respect to your previous comments:
Done. This is now based on the latest version and all changes are against that.
I haven't actually been able to replicate this but I suspect that it is due to too many requests being sent when multiple clients are open. I have implemented a throttling mechanism which means that an update is sent no more than once a second (and in practice less than this) which should fix the issue.
I think the link actually did work but didn't refresh the button in a timely way to make clear that it had worked. I have fixed this by forcing a status refresh on the button as soon as the request is completed which is a much better UX experience.
This was actually quite hard to fix as it is difficult to get links called by javascript to force a new browser window. I have fixed this by changing it to an link which then calls an endpoint in flask to do the redirect. It now works as expected, although I haven't actually been able to roadtest in Android.
Done.
Done.
I've shortened to "Find online" (normal) and "Find" (landscape). I didn't like FMP as I don't think it will be clear to users what that actually is.
Done
I would be grateful for your test, review and comments.
Thanks