Skip to content

Added the possibility to edit showcases #207

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.

Sign up for GitHub

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

Open
NetroScript wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Added the possibility to edit showcases #207

Changes from 13 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
087c0f0
Added the possibility to edit showcases
NetroScript Jul 19, 2018
9b1d40c
Added the possibility to edit showcases
NetroScript Jul 19, 2018
a5a03f1
Fixing potential error
NetroScript Jul 20, 2018
0f9ae26
Removed dependency of param
NetroScript Jul 20, 2018
de4c377
Ensures the array index for the submitted settings.
NetroScript Jul 20, 2018
a8d9f16
Improved readability of `showcaseconfig`
NetroScript Jul 20, 2018
a9264f2
Replaced t with type and added delay to the `ajaxsetshowcaseconfig` r…
NetroScript Jul 20, 2018
72b57d4
Improved behaviour of callbacks when a request to change a single ite…
NetroScript Jul 22, 2018
8c61d78
Fixing indention + removing unnecessary iterator variable
NetroScript Jul 22, 2018
cba563b
Improved timeout for additional changes to showcase config items.
NetroScript Jul 22, 2018
47e6d23
Rewrite from array to object.
NetroScript Jul 22, 2018
1b6364e
Added missing curly braces
NetroScript Jul 22, 2018
bc00349
Check for the maximum number of possible showcases
NetroScript Jul 22, 2018
1773948
fixed ownworkshop
NetroScript Jul 22, 2018
1073e70
Changed name to remainingshowcases + break the loop at that point
NetroScript Jul 22, 2018
0b08247
Removed editing Showcases in editProfile(), added ediShowcaseItem() i…
NetroScript Jul 24, 2018
5e31a54
Removed obsolent variables, changed editShowcaseItem to callback only
NetroScript Jul 25, 2018
0356f54
Removed leftover `this` from Promise, create new object instead of us…
NetroScript Jul 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
283 changes: 276 additions & 7 deletions components/profile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,16 +54,17 @@ SteamCommunity.prototype.editProfile = function(settings, callback) {
}

var values = {};
form.serializeArray().forEach(function(item) {
form.serializeArray().forEach(function (item) {
Copy link
Contributor

@Aareksio Aareksio Jul 22, 2018

Choose a reason for hiding this comment

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

Array.forEach callback is supplied with item index on 2nd parameter

values[item.name] = item.value;
});

for(var i in settings) {
if(!settings.hasOwnProperty(i)) {
var maxshowcases = $(".profile_showcase_selector").length;

for (var i in settings) {
if (!settings.hasOwnProperty(i)) {
continue;
}

switch(i) {
switch (i) {
case 'name':
values.personaName = settings[i];
break;
Expand Down Expand Up @@ -103,15 +104,283 @@ SteamCommunity.prototype.editProfile = function(settings, callback) {
break;

case 'primaryGroup':
if(typeof settings[i] === 'object' && settings[i].getSteamID64) {
if (typeof settings[i] === 'object' && settings[i].getSteamID64) {
values.primary_group_steamid = settings[i].getSteamID64();
} else {
values.primary_group_steamid = new SteamID(settings[i]).getSteamID64();
}

break;

// TODO: profile showcases

case 'showcases':
var numofrequests = 0;

//When supplying a new showcases array, remove the old showcase (order)
Copy link
Contributor

@Aareksio Aareksio Jul 22, 2018

Choose a reason for hiding this comment

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

Whitechars (new lines) please

for (var val in values) {
if (val.indexOf("[") !== -1) {
if (val.split("[")[0] == "profile_showcase")
delete values[val];
}
}

for (var type in settings[i]) {

Copy link
Contributor

@Aareksio Aareksio Jul 22, 2018

Choose a reason for hiding this comment

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

This new line should be above, not below 😉

if(maxshowcases > 0){
Copy link
Contributor

@andrewda andrewda Jul 22, 2018

Choose a reason for hiding this comment

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

Space before ( and after ).

Copy link
Contributor

@andrewda andrewda Jul 22, 2018

Choose a reason for hiding this comment

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

While not super important here, in cases like this it's good practice to break out of your for loop after this condition, i.e.

if (maxshowcases > 0) {
  // ...
} else {
  break;
}

This way, you're not iterating through more times when you know you're not going to use the values.

Copy link
Contributor

@Aareksio Aareksio Jul 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Taking a moment on this - it may be good to invert such statements:

if (maxshowcases === 0) {
  break;
}

// ...

Copy link
Contributor

@andrewda andrewda Jul 22, 2018

Choose a reason for hiding this comment

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

Agreed 👍

Copy link
Contributor

@andrewda andrewda Jul 22, 2018

Choose a reason for hiding this comment

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

Though actually, I guess a more readable approach would be to define a variable currentShowcase and increment it every loop, making sure it's less than maxShowcases. It doesn't make logical sense to be manipulating the value of maxShowcases, whose name suggests it should hold the value of the absolute maximum number of showcases. For the current method, a better name for that variable would be remainingShowcases.


maxshowcases--;

//Variable used to easily make request to`ajaxsetshowcaseconfig` for showcases like trade, items, ...
var showcaseconfig = {
"supplied": false,
"numberofrequests": 0,
"showcasetype": 0,
"itemarray": []
};

//Controls if the callback function is called when a request changing f.e. a single item or game in a showcase fails
var errorcontrol = {
"error": false,
"showerrors": false
};

if (settings[i][type].hasOwnProperty("values")) {
if (settings[i][type]["values"].hasOwnProperty("showshowcaseconfigerrors")) {
errorcontrol["showerrors"] = settings[i][type]["values"]["showshowcaseconfigerrors"];
}
}

switch (settings[i][type].showcase) {

case 'infobox':
values["profile_showcase[8]"] = 8;

if (settings[i][type].hasOwnProperty("values")){
if (settings[i][type]["values"].hasOwnProperty("title")){
Copy link
Contributor

@andrewda andrewda Jul 22, 2018

Choose a reason for hiding this comment

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

Space after ) (same for 158)

values["rgShowcaseConfig[8][0][title]"] = settings[i][type]["values"]["title"];
}
if (settings[i][type]["values"].hasOwnProperty("notes")) {
values["rgShowcaseConfig[8][0][notes]"] = settings[i][type]["values"]["notes"];
}
}

break;

case 'artwork':
values["profile_showcase[13]"] = 13;

if (settings[i][type].hasOwnProperty("values")) {
for (var n = 0; n < 4; n++) {
values["profile_showcase[13][" + n + "][publishedfileid]"] = settings[i][type]["values"][n] || "";
}
}
break;

case 'trade':
values["profile_showcase[4]"] = 4;

if (settings[i][type].hasOwnProperty("values")) {

if (settings[i][type]["values"].hasOwnProperty("notes")) {
values["rgShowcaseConfig[4][6][notes]"] = settings[i][type]["values"]["notes"];
}

if (settings[i][type]["values"].hasOwnProperty("items")) {
showcaseconfig["supplied"] = true;
showcaseconfig["numberofrequests"] = 6;
showcaseconfig["showcasetype"] = 4;
showcaseconfig["itemarray"] = settings[i][type]["values"]["items"];
}
}
break;

case 'items':
values["profile_showcase[3]"] = 3;

if (settings[i][type].hasOwnProperty("values") && settings[i][type]["values"].hasOwnProperty("items")) {
showcaseconfig["supplied"] = true;
showcaseconfig["numberofrequests"] = 10;
showcaseconfig["showcasetype"] = 3;
showcaseconfig["itemarray"] = settings[i][type]["values"]["items"];
}
break;

case 'game':
values["profile_showcase[6]"] = 6;

if (settings[i][type].hasOwnProperty("values")) {
values["rgShowcaseConfig[6][0][appid]"] = settings[i][type]["values"];
}
break;

case 'badge':
values["profile_showcase[5]"] = 5;

if (settings[i][type].hasOwnProperty("values")) {

if (settings[i][type]["values"].hasOwnProperty("style")) {
var styles = ["rare", "selected", null, "recent", "random"];
values["profile_showcase_style_5"] = styles.indexOf(settings[i][type]["values"]["style"]);
}

if (settings[i][type]["values"].hasOwnProperty("badges")) {
var types = ["badgeid", "appid", "border_color"];
for (var n = 0; n < 6; n++) {
for (var t in type){
if (settings[i][type]["values"]["badges"][n] != undefined) {
if (settings[i][type]["values"]["badges"][n].hasOwnProperty(types[t])){
Copy link
Contributor

@andrewda andrewda Jul 22, 2018

Choose a reason for hiding this comment

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

Space after ) (same for 229)

values["rgShowcaseConfig[5][" + n + "][" + types[t] + "]"] = settings[i][type]["values"]["badges"][n][types[t]] || values["rgShowcaseConfig[5][" + n + "][" + types[t] + "]"] || "";
}
}
}
}
}
}

break;

case 'rareachievements':
values["profile_showcase[1]"] = 1;
break;

case 'screenshot':
values["profile_showcase[7]"] = 7;

if (settings[i][type].hasOwnProperty("values")) {
for (var n = 0; n < 4; n++) {
if (settings[i][type]["values"][n] != undefined) {
values["rgShowcaseConfig[7][" + n + "][publishedfileid]"] = settings[i][type]["values"][n];
}
}
}
break;

case 'group':
values["profile_showcase[9]"] = 9;

if (settings[i][type].hasOwnProperty("values")) {
if (typeof settings[i][type]["values"] === 'object' && settings[i][type]["values"].getSteamID64) {
values["rgShowcaseConfig[9][0][accountid]"] = settings[i][type]["values"].getSteamID64();
} else {
values["rgShowcaseConfig[9][0][accountid]"] = new SteamID(settings[i][type]["values"]).getSteamID64();
}
}
break;

case 'review':
values["profile_showcase[10]"] = 10;

if (settings[i][type].hasOwnProperty("values")) {
values["rgShowcaseConfig[10][0][appid]"] = settings[i][type]["values"];
}
break;

case 'workshop':
values["profile_showcase[11]"] = 11;

if (settings[i][type].hasOwnProperty("values")) {
values["rgShowcaseConfig[11][0][appid]"] = settings[i][type]["values"]["appid"];
values["rgShowcaseConfig[11][0][publishedfileid]"] = settings[i][type]["values"]["publishedfileid"];
}
break;

case 'guide':
Copy link
Contributor

@andrewda andrewda Jul 22, 2018

Choose a reason for hiding this comment

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

A lot of these cases contain very similar code. It's hard for me to believe this can't be done in a more efficient way that would be more easily transferable to more showcases as Valve adds them.

Copy link
Author

@NetroScript NetroScript Jul 22, 2018

Choose a reason for hiding this comment

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

I could imagine something considering the possible combinations are (if I am correct)

  • title + notes
  • title
  • array of publishedfileids
  • style and array of badges
  • appid and publishedfileid
  • single value
    and allowing an additional array with single item requests

But assume the readability of this would be terrible compared to the switch case

Copy link
Contributor

@andrewda andrewda Jul 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

There's always a "proper" way to do something, and I think doing it in a way which allows more showcases to be easily added on is a must. I guess the current method isn't too bad, especially after a little cleanup.

In the ideal world, I'd say there should be a Showcases class containing the data that every showcase might want, such as item, title, badge, etc. Then the code here would be able to get passed a specific showcase class from the user and add that to their profile. Maybe this is a little too ambitious for something that probably won't be used too often, but it feels like there's still some work that could be done to make this all more friendly.

values["profile_showcase[15]"] = 15;

if (settings[i][type].hasOwnProperty("values")) {
values["rgShowcaseConfig[15][0][appid]"] = settings[i][type]["values"]["appid"];
values["rgShowcaseConfig[15][0][publishedfileid]"] = settings[i][type]["values"]["publishedfileid"];
}
break;

case 'achievements':
values["profile_showcase[17]"] = 17;

if (settings[i][type].hasOwnProperty("values") && settings[i][type]["values"].hasOwnProperty("achievements")) {
for (var n = 0; n < 7; n++) {
if (settings[i][type]["values"]["achievements"][n] != undefined) {
values["rgShowcaseConfig[17][" + n + "][appid]"] = settings[i][type]["values"]["achievements"][n]["appid"];
values["rgShowcaseConfig[17][" + n + "][title]"] = settings[i][type]["values"]["achievements"][n]["title"];
}
}
}
break;

case 'games':
Copy link
Contributor

@Aareksio Aareksio Jul 22, 2018

Choose a reason for hiding this comment

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

Indent broken here and below

values["profile_showcase[2]"] = 2;

if (settings[i][type].hasOwnProperty("values") && settings[i][type]["values"].hasOwnProperty("games")) {
showcaseconfig["supplied"] = true;
showcaseconfig["numberofrequests"] = 4;
showcaseconfig["showcasetype"] = 2;
showcaseconfig["itemarray"] = settings[i][type]["values"]["games"];
}
break;

case 'ownguides':
values["profile_showcase[16]"] = 16;

if (settings[i][type].hasOwnProperty("values")) {
for (var n = 0; n < 4; n++) {
if (settings[i][type]["values"][n] != undefined) {
Copy link
Contributor

@andrewda andrewda Jul 22, 2018

Choose a reason for hiding this comment

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

This is not the proper way to check if something is undefined in JavaScript. Otherwise you risk an error. Use:

if (typeof foo === 'undefined')

https://stackoverflow.com/a/4726198/5737136

Copy link
Author

@NetroScript NetroScript Jul 22, 2018

Choose a reason for hiding this comment

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

The current way wouldn't cause errors as far as I see it and also allows f.e. null instead of undefined too. Even when using variable !== undefined the variable will always be undefined or defined, because it is a property of an object. Or can you give a example where this specific code would throw an error?

Copy link
Contributor

@andrewda andrewda Jul 23, 2018

Choose a reason for hiding this comment

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

You're right, this shouldn't cause any errors as-is. I think convention is to usually use typeof, though, but it's fine as it is now.

values["rgShowcaseConfig[16][" + n + "][appid]"] = settings[i][type]["values"][n]["appid"];
values["rgShowcaseConfig[16][" + n + "][publishedfileid]"] = settings[i][type]["values"][n]["publishedfileid"];
}
}
}
break;

case 'ownguides':
Copy link
Contributor

@andrewda andrewda Jul 22, 2018

Choose a reason for hiding this comment

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

Why are there two ownguides cases?

Copy link
Author

@NetroScript NetroScript Jul 22, 2018

Choose a reason for hiding this comment

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

My fault, the lower is ownworkshop

values["profile_showcase[12]"] = 12;

if (settings[i][type].hasOwnProperty("values")) {
for (var n = 0; n < 5; n++) {
if (settings[i][type]["values"][n] != undefined) {
values["rgShowcaseConfig[12][" + n + "][appid]"] = settings[i][type]["values"][n]["appid"];
values["rgShowcaseConfig[12][" + n + "][publishedfileid]"] = settings[i][type]["values"][n]["publishedfileid"];
}
}
}
break;
}

if (showcaseconfig["supplied"]) {
for (var n = 0; n < showcaseconfig["numberofrequests"]; n++) {
var requestdata;
if (showcaseconfig["itemarray"][n] != undefined) {
numofrequests++;
requestdata = {
appid: showcaseconfig["itemarray"][n]["appid"],
item_contextid: showcaseconfig["itemarray"][n]["item_contextid"],
item_assetid: showcaseconfig["itemarray"][n]["item_assetid"],
customization_type: showcaseconfig["showcasetype"],
slot: n,
sessionid: values.sessionID
};

setTimeout(self._myProfile.bind(self, "ajaxsetshowcaseconfig", requestdata, function (err, response, body) {

if ((err || response.statusCode != 200) && this.showerrors) {
if (err) {
err.message += " | Happened while updating specific showcase items.";
}

if (callback && !this.error) {
callback(err || new Error("HTTP error " + response.statusCode + " | Happened while updating specific showcase items."));
}
this.error = true;
return;
}


}.bind(errorcontrol)), numofrequests * 1500);
}
}
}
}
}
break;

}
}

Expand Down