Add [Steam] Service #2140
Add [Steam] Service #2140
Conversation
Generated by 🚫 dangerJS |
There was a problem hiding this comment.
Great - thanks for following this up and improving those last two points @TagnumElite 👍
I've left one really minor comment about a variable name, but other that I think this is in good shape. As noted in #2130 (comment) given this is quite a large submission, it would be useful to get a second pass from another maintainer before we merge. Is anyone able to give this a check to make sure we haven't missed anything?
The previous review comments are at #2130 (review) and I've deployed a new review app at https://shields-staging-pr-2140.herokuapp.com/
services/steam/steam.service.js
Outdated
| .required(), | ||
| }).required() | ||
|
|
||
| const collectionFoundOrNoteSchema = Joi.alternatives( |
There was a problem hiding this comment.
Presumably this var name should be
collectionFoundOrNotSchema
There was a problem hiding this comment.
I think I have a few other variables I wish to rename, but I will wait for another reviewer before making any more commits, at the time I will also update the branch
PyvesB
added
the
service-badge
|
|
There was a problem hiding this comment.
👍
Thanks for this PR,
Looks good to me,
Just a couple of notes:
- The second image takes up quite a large amount of space, how do you feel about just having written instructions or something like
right click→Copy Page URL?
- Update the var name @chris48s mentioned.
- Do you think it would also be worth adding the ratings (as a x/5, star rating):
amo/mozilla example:
|
While I am at it, do you wish for me to split the file into more files corresponding to their category? I have updated the var name and a few other ones. It seems the Web API doesn't support ratings, will ask steam (and/or public forum) about this. May I use your picture then because that is a much better, user-friendly, fewer steps approach to getting the URL! |
Sorry not quite sure what you mean by this?
👍
That's fine, we can always add it later in a separate PR once we find a better method to obtain them.
👍 No Problem |
I mean splitting different categories into their own folder i.e:
At the same time renaming the following to be easier to understand:
|
Thanks, I see what you mean now, yeah it may be worth splitting them into separate files, Maybe something similar to these?:
just so it has a little more context as to what part of steam the badge is referring to, in case we add an app/game badge which shows similar info ( Or we could just put the workshop/file badges together in 1 seperate file
Sounds good 👍 |
- Documentation
Create steam-base for base api call Update documentation Fix Variable names and rename most classes for easier to understand context
TagnumElite
force-pushed
the
service-steam
branch
from
f1047e3
to
aaeec0b
Compare
There was a problem hiding this comment.
Anything outstanding for you on this @RedSparr0w or shall we get this merged?
Originally #2130, however, some problems were made with git rebase which caused duplicated commits.
I believe I have addressed all the reported problems with the previous pull request. I do admit I could compress and make this code either more compact or easier to understand, I will do that if everybody needs it.
This pull request adds 7 badges which come the data fetched from Steams Web API ISteamRemoteStorage interface two end points GetCollectionDetails and GetPublishedFileDetails
These seven are:
And once again this pull request resolves #2121
PS: @chris48s Hopefully this is it.