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

RTL plugin lazy loading doesn't work on GeoJSON source #9075

Closed
hyperknot opened this issue Dec 5, 2019 · 9 comments · Fixed by #9091
Closed

RTL plugin lazy loading doesn't work on GeoJSON source #9075

hyperknot opened this issue Dec 5, 2019 · 9 comments · Fixed by #9091
Assignees
Labels

Comments

@hyperknot
Copy link

hyperknot commented Dec 5, 2019

mapbox-gl-js version: 1.60

browser: Chrome latest stable

Steps to Trigger Behavior

  1. Set RTL text lazy loading to true
mapboxgl.setRTLTextPlugin(
  window.location.origin + '/static/js/@mapbox/mapbox-gl-rtl-text/mapbox-gl-rtl-text.min.js',
  err => {
    console.log(err)
  },
  true
)
  1. Load a map without RTL text
  2. Add text which contains RTL characters, for example "این یک متن فارسی است که در سمت راست قرار می‌گیرد"

Expected Behavior

Network panel should load the RTL plugin + the matching PBF files

Actual Behavior

RTL plugin does not get loaded + matching PBF files are not loaded either
Nothing is visible on the map in place of the text.

@ryanhamley
Copy link
Contributor

cc @arindam1993

@arindam1993 arindam1993 self-assigned this Dec 5, 2019
@kkaefer
Copy link
Member

kkaefer commented Dec 6, 2019

@hyperknot looking at the example at https://docs.mapbox.com/mapbox-gl-js/example/mapbox-gl-rtl-text/, right-to-left text seems to work as intended. If you believe it doesn't, please provide a reproduction website (e.g. via [jsfiddle.net]https://jsfiddle.net/)). Please include information about the browser version and operating system used.

@kkaefer kkaefer closed this as completed Dec 6, 2019
@arindam1993
Copy link
Contributor

Hey @kkaefer , I think @hyperknot is referring to lazy loading the plugin, and not the docs example which loads it synchronously.
I think this still might be an outstanding issue with adding rtl text based labels via GeoJSON. Is that what you mean by pt 3. @hyperknot ?

@hyperknot
Copy link
Author

hyperknot commented Dec 9, 2019

@arindam1993 yes, I'm adding text labels via GeoJSON! That's where the whole lazy loading makes sense!

95% of my users are on non RTL regions, hence I would like to make the bundle + plugin loading size as small as possible for them. But at the same time 5% of the users need the RTL plugin, so it'd be important to load it only for those users.

I mean what other use cases can be considered registered which received the recent support in 1.60?

I'll try to make a minimal repro case / test file which you can include in your test suite.

@arindam1993
Copy link
Contributor

arindam1993 commented Dec 9, 2019

I mean what other use cases can be considered registered which received the recent support

The other use case is with vector tiles for the basemap, wherein you start your viewport somewhere without rtl text and then move over to an area with rtl text.

@hyperknot
Copy link
Author

hyperknot commented Dec 9, 2019

I see. So there is indeed a bug, that is: lazy loading doesn't work on GeoJSON sources.

I believe there should be a test case for:

  • GeoJSON which contains RTL text on map load
  • GeoJSON which contains non-RTL text on map load, but is updated to contain RTL text via .setData()

@hyperknot hyperknot changed the title RTL plugin lazy loading broken in 1.60 RTL plugin lazy loading doesn't work on GeoJSON source Dec 9, 2019
@arindam1993
Copy link
Contributor

I can confirm this is indeed the issue, here's a js fiddle with a test case :
https://jsfiddle.net/egyoam5t/3/

@hyperknot
Copy link
Author

@arindam1993 thanks so much for creating the fiddle!

@arindam1993
Copy link
Contributor

arindam1993 commented Dec 10, 2019

#9091 is up which fixes the issue.

Here's a jsfiddle with the latest build that demonstrates that the issue has been fixed.
https://jsfiddle.net/egyoam5t/6/

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

Successfully merging a pull request may close this issue.

4 participants