w3champions / website

The webpage for the www.w3champions.com community project.
46 stars 53 forks source link

refactor(Rankings): add loading status on search request #673

Closed MikalaiLappo closed 1 year ago

MikalaiLappo commented 1 year ago

Expected behavior: while making a request after typing 3+ chars in /rankings search bar, dropdown placeholder text should change to Loading...

Actual behavior: dropdown placeholder text stays No player found while make a request after typing 3+ chars

Header search bar working as expected so I've used its code style

gustav87 commented 1 year ago

Hey! Sorry for the late reply. Why is Playersearch also changed, if this is only concerning /rankings?

MikalaiLappo commented 1 year ago

There are a bunch of components having noDataText method:

  1. components/common/GlobalSearch — if-statements
  2. view/Rankings — if-statements
  3. components/clans/InvitePlayerModal — if-statements
  4. components/common/PlayerSearch — one big ternary chain analogous to the if-statements

I thought it would be better if the thing would be stylisticly consistent Should've make it a separate commit but I do PRs being insomnic

So yeah the Rankings.vue change changes No players found text to Loading... while making a request on /rankings page And the PlayerSearch.vue change is only stylistic

gustav87 commented 1 year ago

I personally prefer the other style :D if you remove the changes in PlayerSearch, I'll approve it!

Note: I agree the same style should be used consistently, but perhaps then we could use a helper method, a Mixin or simply replacing one usage with an already existing component. We have GlobalSearch, PlayerSearch and then some independent searches like in /rankings.

Note 2: I'm biased because I made the PlayerSearch component ;)

MikalaiLappo commented 1 year ago

I've removed the changes in PlayerSearch and now it's just an if for isLoading on /rankings page

Note: I agree the same style should be used consistently, but perhaps then we could use a helper method, a Mixin or simply replacing one usage with an already existing component. We have GlobalSearch, PlayerSearch and then some independent searches like in /rankings.

As I see it: a bunch of components repeat same isLoading and noDataText properties so I'd make a generic Search containing the stuff being repeated and within the bunch of components replace the properties with Search already containing it

Like a wrapper component around v-autocomplete which would contain the shared properties but it must be decoupled from any source of data and handle some actions through callbacks. Like PlayerSearch is a wrapper around v-autocomplete but it's tied to adminStore

A possible way I'd solve it in React is here There are like:

  1. GenericSearchHOC — handles input (min length, isLoading, no result) -> calls back parent's method passed within props with the input value
  2. PlayerSearch — contain state and fetch/storage handler methods -> makes a GenericSearchHOC with fetch handler as a callback props and also puts data-based JSX inside (which would be an empty array initially) as props.children -> input triggers fetch/storage handler (i.e. retrieving data and updating PlayerSearch state with it) -> PlayerSearch makes new JSX based on the new data and passing it into the GenericSearchHOC
  3. So like PlayerSearch ends up not caring about how to handle input and GenericSearchHOC is not coupled to data sources just calling callbacks

It's highly likely that I'm missing something since I never actually touched Vue and have no idea about project`s current data flow

I was just searching for Happy's smurf accounts on the website and precocious No player found annoyed me making me think I've mistyped something so accepting that one if-statement PR would make me happier

gustav87 commented 1 year ago

Thanks! You can join us at Discord if you haven't already. https://discord.gg/hRCa6DcB

We have a new React project (currently a private repo) if you feel like contributing some more! Feel free to message me, Kovax, on Discord.