-
Notifications
You must be signed in to change notification settings - Fork 192
implement Fonts service #416
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
base: main
Are you sure you want to change the base?
Conversation
|
|
By the way, I needed this for https://gist.github.com/andrewharvey/ff5e2c15b8a60249a593dc189192eb02 which reports
|
|
@katydecorah interested in reviewing? |
|
👋 @andrewharvey I shared this PR with the team that works on Fonts API and they'll have someone take a look (hopefully this week)! |
jseppi
left a comment
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.
Thanks for this contribution, @andrewharvey! The one bug I found during review is that the verb used in the putFont method should be POST. The rest of my comments are suggestions around consistency and usability.
Also, I see that there's an existing font method in the styles client: Styles.getFontGlyphRange -- would you mind deleting that one and the associated tests since this PR puts that method in a more appropriate place?
services/fonts.js
Outdated
| * const newFont = response.body; | ||
| * }); | ||
| */ | ||
| Fonts.putFont = function(config) { |
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.
For consistency with how this method is documented in our public docs, could you rename the method to addFont? https://docs.mapbox.com/api/maps/fonts/#add-a-font
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.
In docs/development.md it says to use "create" for POST requests, do you think it should still be addFont or createFont?
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.
👍 createFont is a good suggestion for consistency
| start: v.required(v.number), | ||
| end: v.required(v.number), |
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.
Since we know that end is always start + 255, we could make this method a little more ergonomic by only requiring the start parameter. Then, we'd just add 255 to it to come up with the correct fileName:
var fileName = config.start + '-' + (config.start + 255) + '.pbf';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.
Must end always equal start+255 or could users still request a larger slice?
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.
Yup, end must always be start + 255
Co-authored-by: James Seppi <james.seppi@mapbox.com>
| Fonts.updateFontMetadata = function(config) { | ||
| v.assertShape({ | ||
| font: v.required(v.string), | ||
| visibility: v.required.oneOf(['public', 'private']), |
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.
Ah, sorry I got the syntax wrong in my review suggestion, @andrewharvey, which is why tests are failing. Should be:
v.required(v.oneOf(['public', 'private']))
https://docs.mapbox.com/api/maps/fonts/
getFontGlyphRangestill exists in Styles, which I suggest we do for backwards compatibility. However this means this method is now duplicate in the code base. So we can either: