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

Upgrade getPlayerThumbnail() to support bust and headshot #274

Merged
merged 5 commits into from
Mar 26, 2021

Conversation

alanbixby
Copy link
Member

Currently the library has no method to retrieve player headshots. The endpoint used by getPlayerThumbnail() is nearly identical to the headshot endpoint, so I have added an optional fifth parameter to retrieve headshots.

Minimal code clean up and increased specificity to typing are included. All changes are backwards compatible with the old implementation.

Copy link
Member

@suufi suufi left a comment

Choose a reason for hiding this comment

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

image

So there's an issue with the two endpoints. The headshot one has a different set of sizes. I think we would have to do something similar to lib/group/getLogo.js line 14 and have a mixed parameter (e.g. {ThumbnailSizes|HeadshotSize}). While on the topic, I think we'd also be able to include the bust thumbnail as well.

@alanbixby
Copy link
Member Author

alanbixby commented Mar 25, 2021

I rewrote getPlayerThumbnail() so now it supports Body, Bust and Headshot thumbnails. Tldr is changes add bust and make error handling easier for the end user.


A quirk about the endpoint is that uncached users (Roblox's cache) and users wearing moderated assets will return objects of status 'Pending' and 'Blocked' respectively and have imageURL set to null. As such, I've added a config to settings.json, and a folder in img to point the null thumbnails to https://noblox.js.org/moderatedThumbnails/moderatedThumbnail_{size}.png.

Pending responses tend to resolve themselves with a follow-up request, so getPlayerThumbnail() recursively calls itself up to settings.thumbnail.maxRetries (2) additional times to resolve pending thumbnails. If this fails, then it falls back to the noblox.js.org thumbnail link, or a URL that the user can specify in settings.json.


For the size parameter, users can provide a number or a string for the dimension. 150 <-> "150x150"; if no size is provided it defaults to the largest possible size, and if an invalid size is provided, the error contains the array of valid sizes.

Documentation in index.d.ts uses type definitions of BodySize, BustSize, and HeadshotSize, while the JSDocs documentation will only state 'number | string`. My reasoning is the type definitions don't translate well to the website's documentation, and instead we can provide a link to this table in the parameter description.


image

@alanbixby alanbixby requested a review from suufi March 25, 2021 05:57
@alanbixby alanbixby changed the title Add optional headshot parameter to getPlayerThumbnail Upgrade getPlayerThumbnail() to support bust and headshot Mar 25, 2021
@suufi suufi merged commit 113b1a3 into noblox:master Mar 26, 2021
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.

2 participants