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

WebHost: Fix setup guide link not working for games with special characters #3269

Merged
merged 3 commits into from
May 16, 2024

Conversation

NewSoupVi
Copy link
Collaborator

@NewSoupVi NewSoupVi commented May 5, 2024

"Mario & Luigi - Superstar Saga" contains the character &.

The DOM element for it on the "Archipelago Guides" page has the id Mario%20%26%20Luigi%20Superstar%20Saga. This is because tutorialLanding.js line 26 uses encodeURIComponent(game.gameTitle), which escapes special uri characters.
image

However, the link on the supported games page goes to /tutorial/#Mario%20&%20Luigi%20Superstar%20Saga.
image

This is because supportedGames.html just directly uses #{{ game_name }}.

This causes the autoscroll down to the correct setup guide not to work when clicking on this link.

Using #{{ game_name|urlencode }} makes it work as expected.

@github-actions github-actions bot added affects: webhost Issues/PRs that touch webhost and may need additional validation. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels May 5, 2024
@NewSoupVi NewSoupVi added the is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. label May 5, 2024
@NewSoupVi
Copy link
Collaborator Author

Not actually sure whether this is the best way to do this, but it does work.

Copy link
Collaborator

@remyjette remyjette left a comment

Choose a reason for hiding this comment

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

Looks good to me! Tested and it works as expected now. In my opinion this is the correct approach - if we're generating the IDs in tutorialLanding.js by url-encoding it, it makes sense to also do so when generating links to it.

@black-sliver
Copy link
Member

I think
url_for("tutorial_landing", _anchor=game_name)
would be the proper way to do this

@remyjette
Copy link
Collaborator

remyjette commented May 5, 2024

url_for doesn't encode the url, so that would give the same bug as we have today.

To use _anchor we would have to do something like

            {% set anchor = game_name|urlencode %}
            <a href="{{ url_for("tutorial_landing", _anchor=anchor) }}">Setup Guides</a>

@NewSoupVi
Copy link
Collaborator Author

NewSoupVi commented May 7, 2024

url_for doesn't encode the url, so that would give the same bug as we have today.

To use _anchor we would have to do something like

            {% set anchor = game_name|urlencode %}
            <a href="{{ url_for("tutorial_landing", _anchor=anchor) }}">Setup Guides</a>

Confirmed that this is correct, in which case I would rather keep this PR as is, rather than doing it like in this code snippet.

@black-sliver
Copy link
Member

url_for(..., _anchor = game_name | urlencode)

@remyjette
Copy link
Collaborator

Huh. For some reason I didn't think you could use filters inside an argument list. TIL. Yeah, that works. Do that.

@NewSoupVi
Copy link
Collaborator Author

url_for(..., _anchor = game_name | urlencode)

Sure, I guess I didn't care to mess with the structure of the code but I can do that if you think it's better

Copy link
Member

@black-sliver black-sliver left a comment

Choose a reason for hiding this comment

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

Yeah, imo moving the anchor to url_for is a lot better than mixing "plain text" in.

I have not tested the final version here, but I suspect you did.

@LegendaryLinux LegendaryLinux merged commit 467bbd7 into main May 16, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: webhost Issues/PRs that touch webhost and may need additional validation. is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants