Skip to content
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.

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

Optional gameId/seasonId in routes and redirection #874

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

Posoroko
Copy link
Contributor

Issue number

Relevant issue number

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

Copy link
Contributor

@itsalaidbacklife itsalaidbacklife left a comment

Choose a reason for hiding this comment

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

Right idea, but this prevents initial navigation to the stats page (before season ids are downloaded).

This issue was just closed, coincidentally, so I'll close this PR. Sorry for the miscommunication!

src/router.js Show resolved Hide resolved
@itsalaidbacklife
Copy link
Contributor

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)

@Posoroko
Copy link
Contributor Author

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

@Haviles04
Copy link
Contributor

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?

@Posoroko
Copy link
Contributor Author

Posoroko commented Feb 1, 2024

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"

@Haviles04
Copy link
Contributor

Haviles04 commented Feb 1, 2024

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 Haviles04 added version-patch An update that warrants a bumping the project's patch version (e.g. 4.0.0 => 4.0.1) frontend Requires changes to the frontend (vue) client labels Feb 2, 2024
@itsalaidbacklife
Copy link
Contributor

@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?

@Haviles04
Copy link
Contributor

@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

@itsalaidbacklife itsalaidbacklife mentioned this pull request Feb 15, 2024
4 tasks
@@ -104,7 +104,7 @@ const routes = [
},
},
{
path: '/stats/:seasonId?',
path: '/stats/:seasonId',
Copy link
Contributor

Choose a reason for hiding this comment

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

this should have the ? this way you can see the stats page with or without a seasonId

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend Requires changes to the frontend (vue) client version-patch An update that warrants a bumping the project's patch version (e.g. 4.0.0 => 4.0.1)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: /Game and /Spectate bring you to blank screen
3 participants