cuttle icon indicating copy to clipboard operation
cuttle copied to clipboard

Optional gameId/seasonId in routes and redirection

Open Posoroko opened this issue 2 years ago • 7 comments

Issue number

Relevant issue number

  • Resolves #871

Please check the following

  • [ ] Do the tests still pass? (see Run the Tests)
  • [ ] Is the code formatted properly? (see Linting (Formatting))
  • For New Features:
    • [ ] Have tests been added to cover any new features or fixes?
    • [ ] Has the documentation been updated accordingly?

Please describe additional details for testing this change

-made the gameId and seasonId optional in the routes that should contain it. (lobby, game, spectate and rematch, stats) -redirection to home if no gameId/seasonId is found in the route path

Posoroko avatar Jan 28 '24 07:01 Posoroko

Upon a second look, I think this is appropriate for the /game, /lobby and /spectate routes, just not for /stats, because stats downloads the season ids on created().

In that case I think it makes sense to update the helper function to specifically look for the gameId param (and to rename it accordingly)

itsalaidbacklife avatar Jan 28 '24 15:01 itsalaidbacklife

@itsalaidbacklife yep, ok. I'll do the changes. 👍

Posoroko avatar Jan 28 '24 21:01 Posoroko

So, after looking at the your code and noticing you added the ? to make the param optional it made me think that maybe we shouldn't redirect to the home page. If this will still load the GameView.vue but it will show the GameUnavailableDialog it might be better that way. This way the end user has an explicit error message stating what's going on, instead of just a redirect which could potentially confuse them.

@Posoroko @itsalaidbacklife what are your thoughts?

Haviles04 avatar Jan 29 '24 21:01 Haviles04

This way the end user has an explicit error message stating what's going on, instead of just a redirect which could potentially confuse them.

@Haviles04

About the possible confusion... If I have this right, this redirection would only occur if the user manually enter a url with no param. Wich is not really the expected behaviour. So weird results when trying to "hack" the router is probably not so problematic.

Could we redirect home and get the router to throw an error to be displayed if the gameId is missing ? something like : "Please provide a game id or select a game in the list"

Posoroko avatar Feb 01 '24 13:02 Posoroko

This way the end user has an explicit error message stating what's going on, instead of just a redirect which could potentially confuse them.

@Haviles04

About the possible confusion... If I have this right, this redirection would only occur if the user manually enter a url with no param. Wich is not really the expected behaviour. So weird results when trying to "hack" the router is probably not so problematic.

Could we redirect home and get the router to throw an error to be displayed if the gameId is missing ? something like : "Please provide a game id or select a game in the list"

So by just adding the optional param to /game will still load the game view(p reviously it was just a white screen) but without the gameId it should display the GameUnavailable overlay that displays an error message with a "Go home" button. It's technically less logic for a mistake that likely won't occur often while still giving an message..

I feel like if someone puts the link in they won't know what went wrong if they're just automatically redirected

Haviles04 avatar Feb 01 '24 13:02 Haviles04

@Posoroko sorry it's taken me so long to get back to a review here! I've been hellbent on wrapping up some changes and then coordinating the logistics of the Cuttle World Championship last Saturday and now that we're through that, I have bandwidth to dig back into QoL improvements like this.

I agree with @Haviles04 that it would be ideal to show the GameUnavailableView when the game id is missing so you can see that something is wrong and hit the button to go home. I think we could do that by adding a route mapping in the router for when you have /game without a /:gameId that points to the GameUnavailableView. Does that make sense?

itsalaidbacklife avatar Feb 15 '24 17:02 itsalaidbacklife

@itsalaidbacklife @Posoroko I think the ? On the gameID is enough actually.. it'll load gameView but the store won't have an id which will trigger the dialog

Haviles04 avatar Feb 15 '24 17:02 Haviles04

Closing as stale

itsalaidbacklife avatar Jun 28 '24 17:06 itsalaidbacklife