Skip to content

better readability and scalability in bar graphs with differing numbers of matches#125

Open
AetheriumSlinky wants to merge 1 commit intomainfrom
MK/player_commander_trends_max_y_fix
Open

better readability and scalability in bar graphs with differing numbers of matches#125
AetheriumSlinky wants to merge 1 commit intomainfrom
MK/player_commander_trends_max_y_fix

Conversation

@AetheriumSlinky
Copy link
Copy Markdown
Contributor

Problem: some players have 20 matches, some have 200. The bar graph showing their placements (and the general one in Match Trends) had a fixed Y-maximum which made some graphs look very flat and somewhat unreadable.

Solution: this fix changes the maxY value of each bar graph to scale with the tallest column (and adds +5 to it) to make the graphs more readable.

return { x: Number(numberOfTurns), y: matchesLengthDictionary[numberOfTurns] };
});

const matchLengthsMaxY = Math.max(...matchesWithLengthsData.map(height => height.y)); // Finds the tallest column
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

your logic here works, but it is doing a little bit extra work than required.

Can we instead compute the maxY inside the .map on line 22? Basically initialize the "max" to the first value of the array, and everytime you loop to a new value in the .map, you can do a comparison to the existing max, and replace it if it is greater :)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The same goes for your other usages :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm going to be using .reduce?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

in this case, no. ".map" is already looping through every element of matchesWithLenghtsData. The "iterator" or current value of each iteration is "numberOfTurns". If you do "matchesLenghtsDictionary[numberOfTurns]" you'll get the "y" value aka the match length for that particular match.

you can save that in a variable that is keeping track of your max, updating the max as you iterate AND find a new length that is larger than your existing saved max.

@davidpchi
Copy link
Copy Markdown
Owner

Screen caps of the before and after please!

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