maproulette / maproulette3

MapRoulette, the micro-tasking tool for OpenStreetMap
https://maproulette.org
MIT License
128 stars 35 forks source link

Improve error messages when Rapid editor fails to load #2441

Closed jake-low closed 1 month ago

jake-low commented 2 months ago

Previously if Rapid failed to load from the CDN, a user would see a message that the Rapid editor didn't support their browser, which was confusing. The text was also rendered inside the iframe, so it displayed in the browser default style (black, serif), which is hard to read against MapRoulette's dark green background.

This PR changes the way the iframe initializes. The parent now has to call iframe.contentWindow.setupRapid(), which returns a promise. The promise normally resolves with a Rapid editor context. If it rejects, it provides an error message that tells the user what went wrong. The RapidEditor component has been adjusted to call this method and display any error that it returns to the user. I've made sure to return different error messages in the case where Rapid fails to load from the CDN vs when it loads but then detects that the user's browser is not supported.

I've also changed the way Rapid's JavaScript bundle and CSS stylesheet are loaded, in order to deduplicate the occurrences of the Rapid CDN URL to a single constant definition. This will make it easier to implement #2438, although there are still some more problems to solve before that can be completed (I left a source code comment with a bit more details on this).

CollinBeczak commented 1 month ago

this looks good, if you want to move forward with the environment variable implementation, you should be able to do this: https://github.com/maproulette/maproulette3/compare/jlow/rapid-editor-better-errors...CollinBeczak/rapid-editor-better-errors

jake-low commented 1 month ago

Thanks. I considered that approach but decided to leave things simple for now, since long term I'm still working on migrating away from using react-scripts / Create React App, which would give us more flexibility in where we can use compile-time constants.

CollinBeczak commented 1 month ago

makes sense, than yeah, looks good to me!