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

Add links to tile provider authentication docs #914

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

ianthetechie
Copy link
Contributor

@ianthetechie ianthetechie commented Jul 26, 2024

This probably confused some people in the past, since vector tiles won't even display an access denied image ;)

Before (no information on access keys and where to get them):

image

After (add links to providers + add a field for Stadia Maps):

image

@zstadler
Copy link
Collaborator

Should there also be a "Stadia Maps Access Token" field?

@ianthetechie
Copy link
Contributor Author

Should there also be a "Stadia Maps Access Token" field?

It's not required for most web browser uses; I'll add one either this weekend or Monday.

I see I also missed updating(?) some tests. Will look at that too.

@nyurik
Copy link
Member

nyurik commented Aug 21, 2024

@HarelM any blockers for this one?

@HarelM
Copy link
Collaborator

HarelM commented Aug 21, 2024

Yes, translation is now needed for text.
Also the above comment about adding an input for the token was not addressed.

@ianthetechie
Copy link
Contributor Author

Got side tracked on a few other projects, but got back to adding the token support in the latest edits.

Thanks for letting me know about the translation @HarelM! I didn't know that was a thing 😅 I've also added a few lines explaining that to the README.

src/libs/style.ts Outdated Show resolved Hide resolved
@HarelM
Copy link
Collaborator

HarelM commented Aug 28, 2024

Yes, translation was added very recently.
Other than the above, this looks good.
It would be great if you can update the initial post with "before" and "after" screenshots.

@HarelM
Copy link
Collaborator

HarelM commented Aug 28, 2024

Thanks!
Should there be an input for stadia maps api key?

@HarelM
Copy link
Collaborator

HarelM commented Aug 28, 2024

Wait, what about the translation of the text you added?

@ianthetechie
Copy link
Contributor Author

ianthetechie commented Aug 28, 2024

Ack.

  1. I didn't update the "after" screenshot.
  2. I think I didn't add the i18n bits for top-level help text I added (it didn't exist when I added that?

I'll address both momentarily.

@ianthetechie
Copy link
Contributor Author

Hmm... I wonder if it is easier to localize these as separate help text strings... I don't know the grammatical structure of Hebrew, but I imagine there is some language where it might be complicated to do this all correctly.

Thoughts on moving it out of the single header text to the expandable help? Here's what it looks like now:

image

@ianthetechie
Copy link
Contributor Author

Ick; that's going to be fun... Sorry I'm not much of a frontend dev, but it looks the HTML is being escaped if I try specifying strings this way...

image

So it's going to be a bit of work with either approach :/

@HarelM
Copy link
Collaborator

HarelM commented Aug 28, 2024

I think adding a description like other fields is a good idea.

@ianthetechie
Copy link
Contributor Author

Okay. This will take some effort to untangle (for me at least). Even adding a relatively simple "learn more" link along with the help text is proving to be difficult.

image

I was able to get this far with it, but hit two roadblocks:

  1. I can't figure out how to get a t function down to translate the learn more link text.
  2. The link is valid, and it fires an onClick handler, but left click doesn't work. I assume something in the hierarchy is preventing propagation.

@@ -130,6 +130,12 @@ class ModalExportInternal extends React.Component<ModalExportInternalProps> {
value={(this.props.mapStyle.metadata || {} as any)['maputnik:thunderforest_access_token']}
onChange={this.changeMetadataProperty.bind(this, "maputnik:thunderforest_access_token")}
/>
<FieldString
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would recommend, instead of using the FieldString to maybe use the react components that this filed is built from, this way you'd have more control, and there won't be a need to add something specific to Doc component, which is a genereal one.
This is just a though...

Copy link
Collaborator

Choose a reason for hiding this comment

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

In order to pass down t you simply need to pass it down to the relevant component, but since doc is part of block which is part of fieldsstring (IIRC) you'll need to pass it down a lot of levels, which is not trivial (and hackish).
Another possibility is translating it in the upper part and passing it down.
You might be able to construct a React Element and pass it down instead of passing down only a string, but that will require some typings updates in order to achieve this, and thay might not be trivial as well.
Let me know if I can help in any way.

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.

4 participants