Skip to content

Conversation

@StrangeRaptor
Copy link
Contributor

Changelog:

  • Replaced const localVerdict within src/components/TF2/Player/playerutils.tsx with const LOCAL_VERDICT_OPTIONS from new file playerConstants.tsx
  • Created new file src/constants/playerConstants.tsx containing the following:
    • export type VERDICT_TYPES which can be one of 6 strings (based on what I could find this value can be)
    • interface VERDICT_OPTION with structure { label: string, value: VERDICT_TYPES | string T}
    • export const LOCAL_VERDICT_OPTIONS: VERDICT_OPTION[] (this is effectively just const localVerdict hence why it replaces it but using types)
  • Created new component src/components/TF2/ScoreboardTable/SortableTableHeader.tsx that renders header as a button with a sort icon that reflects current sorting state
  • Created new file src/constants/tableConstants.tsx containing the following:
    • export enum SORT_TYPES with values:
      • UNSORTED
      • SORT_ASC
      • SORT_DESC
    • export enum SORT_OPTIONS with values:
      • SORT_BY_USER
      • SORT_BY_TIME
      • SORT_BY_RATING
    • export interface TableHeaderSorting with structure { sortValue: SORT_OPTIONS, sortType: SORT_TYPES }
    • export interface SortableHeader with structure { sortValue: SORT_OPTIONS, nameKey: string, hideWhenSmall?: boolean } where name key is what is passed for translation purposes
    • export const DEFAULT_HEADER_SORT: TableHeaderSorting set to unsorted and sort by user
    • export const SORTABLE_SCOREBOARD_HEADERS: SortableHeader[] containing the 3 possible sorting options
    • export const RATING_SORT_ORDER which uses VERDICT_TYPES to make a simple way to define the "Rating" column is sorted via numbers
  • Updated src/components/TF2/ScoreboardTable/ScoreboardTable.tsx to use new <SortableTableHeader /> and constants from tableConstants.tsx to define sorting functionality. In all sorting cases, the current user is displayed at the top of the team's list
  • Updated tailwind.config.js gridTemplateColumns so the new <SortableHeader /> won't be weirdly squished or have the sorting icon off screen

@megascatterbomb
Copy link
Contributor

Comments on functionality:

  • I think it's unintuitive that each team can be sorted in different ways. I would suggest unifying the sort method across that whole page.
  • The ascending/descending default is inconsistent. Changing the sort type should always default to one or the other i.e. not be dependent on what the user selected previously.
  • The default sort method by kills can probably be deprecated; we can discuss what should be the default, but it probably shouldn't be kills as that tends to be unstable, and once you choose something else you can't go back to sorting by kills.
  • I would find it useful if there was an option to force disconnected players to be sorted to the bottom. This can be added to the preferences menu; it doesn't need to take up space on the main screen.
  • It would be useful to persist the chosen sort options within the user's preferences.
  • Nitpick: The click-area for the "User" header is massive. If you accidentally click a bit left of the "Time" header you might sort by username instead.
  • BUG: Sorting by "Rating" doesn't appear to properly account for the "You" verdict.
    image

Overall a good start, just needs some refinement.

@Bash-09
Copy link

Bash-09 commented Jan 25, 2024

This is great, I would love to be able to sort by ways other than kills. Thanks for adding this functionality :)

I agree with mega that default by kills should be changed, as it doesn't accurately reflect the in-game scoreboard which uses additional information to get the actual score. I'm personally in favour of time, as then all the bots and cheaters that newly join a game will consistently hang around one end of the board instead of flying through it as they go and kill everything in sight.

Being able to sort by kills would certainly still be useful to have though, hence I greatly appreciate this feature.

@megascatterbomb
Copy link
Contributor

For the sorting methods that aren't based on the existing columns, you can add sort buttons alongside the extra details that appear when you click the player. Keeps them out of the way for when they're not desired.
image

@StrangeRaptor
Copy link
Contributor Author

* The ascending/descending default is inconsistent. Changing the sort type should always default to one or the other i.e. not be dependent on what the user selected previously.

In regards to this: what do you mean? Do you mean that when sorting by clicking the header that the user should be able to have multiple sorted? For example: sort by desc on both user and time instead of only being able to have time or user?
If this is the case I am unsure how this will work functionally but I also feel like I am misunderstanding what is meant here either way.

* The default sort method by kills can probably be deprecated; we can discuss what should be the default, but it probably shouldn't be kills as that tends to be unstable, and once you choose something else you can't go back to sorting by kills.

I think with this I'll set it to sort by time for now, with the most recent appearing at the bottom since I think this makes a bit more sense practically but either way the default is designed to be changed easily so this can just be how it is for now and if later it needs to be changed it can be done very easily.

* It would be useful to persist the chosen sort options within the user's preferences.

I think this is a great idea but I figure to ask: should there also be a preference to have the current user always display at the top regardless of sorting? Currently this is just how it is but wondering if this should be an option that is default enabled and can be turned off otherwise

* BUG: Sorting by "Rating" doesn't appear to properly account for the "You" verdict.

I have been unable to replicate this so far. The only thing that I think should/would be causing this is if somehow the isSelf flag is returning true for the bots listed as in testing with fake data, this problem does not appear. If possible could you pass me the data that ends up being passed here? There is definitely something I am missing here

image

@StrangeRaptor
Copy link
Contributor Author

* BUG: Sorting by "Rating" doesn't appear to properly account for the "You" verdict.

I have been unable to replicate this so far. The only thing that I think should/would be causing this is if somehow the isSelf flag is returning true for the bots listed as in testing with fake data, this problem does not appear. If possible could you pass me the data that ends up being passed here? There is definitely something I am missing here

Scratch this. Found a way to kind of replicate it with sorting by time as default, I will update in the next commit and hopefully this should fix it

…ult sort to time, refactor sorting to be in a separate utils file
Copy link
Contributor

@SammCheese SammCheese left a comment

Choose a reason for hiding this comment

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

It seems like the sorting does not apply to the BLU player list, no matter which sort option is used?

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.

4 participants