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

Expose mapboxgl.rtlTextPluginRequested boolean #8585

Closed
wants to merge 2 commits into from

Conversation

ryanhamley
Copy link
Contributor

Launch Checklist

Fixes #7869

  • briefly describe the changes in this PR
    This PR exposes mapboxgl.rtlTextPluginRequested which returns a boolean that is true if the RTL plugin has previously been requested. This allows users to avoid requesting the plugin multiple times which causes an error.
  • document any changes to public APIs
  • manually test the debug page
    Added an RTL debug page

I considered exposing the existing clearRTLTextPlugin method, but ultimately decided that this made more sense code-wise. I'm open to differing opinions though.

if (!mapboxgl.rtlTextPluginRequested) {
  mapboxgl.setRTLTextPlugin(url);
}

@@ -7,6 +7,7 @@ toc:
- supported
- version
- setRTLTextPlugin
- rtlTextPluginRequested
Copy link
Member

Choose a reason for hiding this comment

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

Note that this file is being moved over to https://github.com/mapbox/mapbox-gl-js-docs — should we keep it in this repo? We could either keep it, or move it over but PR ToC changes to the docs repo after a release separately. cc @colleenmcginnis

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm willing to defer to you and Colleen on whether this file should be moved or not since you have both thought about this way more than I have. This isn't super urgent so I can wait until we get the docs separated in order to merge this PR. That gives us time to figure out what we want to do with the ToC.

Copy link
Contributor

Choose a reason for hiding this comment

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

should we keep it in this repo? We could either keep it, or move it over but PR ToC changes to the docs repo after a release separately.

It would be better to move this over to docs, and add a step to the post-release PR for docs repo to ensure that these types of changes get added to the ToC.

Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

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

The rtlTextPluginRequested name does not seem intuitive to me. What is difficult to understand is who has requested the plugin, is it that setRTLTextPlugin was called once, or that gl-js requested this plugin because it found RTL text.

An alternative would be to expose the status of the plugin - being one of : unavailable, pending/loading, available/loaded, error and an event handler to get notified of status change. One reason for this is that the plugin can go from loading/pending to error.

The

@ryanhamley
Copy link
Contributor Author

ryanhamley commented Aug 5, 2019

I'm not sure if an event listener would really solve the use cases that were reported for this feature. A typical use case is something like a React component which always tries to initialize the plugin whenever a user navigates to a route that uses that component. An event listener would theoretically be cleaned up and removed each time the component is removed. Even if you persisted the event listener, once the plugin is instantiated, the events wouldn't be firing anymore so the component wouldn't always be receiving the event to alert it if the plugin is available or not. We need a more active way of checking the plugin's availability.

An option that I also considered that we could do instead of or even in tandem with this PR is to expose the clearRTLTextPlugin method which could be called when cleaning up a component or route which would make re-initializing the plugin safe the next time the component is instantiated.

@asheemmamoowala
Copy link
Contributor

'm not sure if an event listener would really solve the use cases that were reported for this feature.

@ryanhamley the event handler was a suggestion in addition to a status check API:

to expose the status of the plugin - being one of : unavailable, pending/loading, available/loaded, error

Would that not satisfy the reported issue?
Your sample code would then change to:

if (mapboxgl.rtlTextPluginStatus === 'unavailable') {
  mapboxgl.setRTLTextPlugin(url);
}

@ryanhamley
Copy link
Contributor Author

This is fairly out of date so I'm closing in favor of a slightly different implementation #8864

@ryanhamley ryanhamley closed this Oct 11, 2019
@ryanhamley ryanhamley deleted the check-rtl-was-requested branch October 11, 2019 23:45
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.

Expose a "clearRTLTextPlugin" or a give a way to check if the rtl plugin is requested
4 participants