-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Map Block: Mapbox Key Input Improvements #28730
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works well. The biggest issue I have are the non-localized strings, everything else is minor.
Definitely true. This is largely because of the recent change to camelCase attributes. The newly renamed attributes don't match the saved block, hence the error. This was the main reason I wanted to get the camelCase PR merged first - to avoid lots of broken blocks with each new branch. |
…r failed attempt to change API key do not revert to initial screen since a valid API key is still in use for the map.
…lds to the previous, good token.
5624e2a
to
3ae573d
Compare
Nice spot. Fixed in e2acff7 |
The copy is good to go after your most recent change. Nice work! |
} ); | ||
}, | ||
result => { | ||
this.onError( null, result.message ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it normal that we always call this.onError
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.onError
should only be called if the API call fails. Is that what you're asking about?
<RawHTML>{ getAPIInstructions }</RawHTML> | ||
<p>{ __( 'To use the map block, you need an Access Token.' ) }</p> | ||
<p> | ||
<ExternalLink href="https://www.mapbox.com"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we linked users directly to https://www.mapbox.com/account/ instead.
They would see a login page asking them to create an account.
But the access token would be easier to find.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MichaelArestad what do you think?
The code looks great and it tested well. I wonder if we could update the styles so that the contact form and the maps bock have similar styles when it comes to the place holder. cc: @MichaelArestad , @mapk |
@enejb I think there's a definite need for a common pattern for these. We will be exploring that in Phase 2, |
Changes proposed in this Pull Request
This PR improves the initial screen of the Map block, before user has input their Mapbox key, in the following ways:
Access Token
which is Mapbox's language vs. Google'sAPI Key
. This comes courtesy of @oskosk.Testing instructions
Create Map block, add key, verify flow. After key has been accepted, in sidebar set to an invalid key and UPDATE TOKEN. You should see error message but the token reverts to last good value and block functions. Verify copy changes and spelling, too.
JN link: https://jurassic.ninja/create/?gutenberg&gutenpack&shortlived&jetpack-beta&calypsobranch=update/map-key-management&branch=master