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

feat(forums): rewrite viewtopic page in React #3205

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

wescopeland
Copy link
Member

This PR rewrites the viewtopic.php in React. Like other PRs involving shortcodes, viewtopic.php and the new React UI coexist on this branch, with the intention of removing viewtopic.php in an immediate followup.

How to test this PR:

http://localhost:64000/viewtopic.php?t=2202
http://localhost:64000/forums/topic/2202

In the follow-up PR, I will also add comprehensive redirect logic to ensure old URLs link to the correct new route.

Behaviors to verify:

  • Viewing topics.
  • Replying to topics (as well as previewing replies).
  • Being locked out of a topic that has higher minimum perms than your account.
  • Changing a topic's title.
  • Setting minimum perms on a topic.
  • Deleting a topic.
  • The ?page query param.
  • Directly linking to a post in a topic.
  • Edit button visibility (for moderators and non-moderators).
  • Ensuring timestamps on posts respect the user's absolute dates pref.
  • Toggling the user's topic subscription status.
  • Pagination.

All of this logic should be covered by tests, but it's all probably worth a routine spot check. For the time being, I tried to do as close to a 1:1 migration as I could, avoiding any major behavior changes.

Before
Screenshot 2025-02-10 at 7 19 42 PM

After
Screenshot 2025-02-10 at 7 19 14 PM

Copy link
Member

Choose a reason for hiding this comment

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

Something about http://localhost:64000/forums/topic/28943?page=2 is causing it to not render.
image

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like there is a very broken [url] embed in a post on that page which causes the ShortcodeUrl component to explode. I've added some error handling to account for this in latest. It is a behavior change from the PHP UI, but those links just lead to 404s in the PHP UI.

Screenshot 2025-02-23 at 3 03 14 PM

>
{t('Edit')}
</a>
) : null}
Copy link
Member

Choose a reason for hiding this comment

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

[url=] at end of [spoiler] is falling outside of spoiler.
[url=] in [spoiler] is not being evaluated.

http://localhost:64000/viewtopic.php?t=29236&c=262386#262386
image

http://localhost:64000/forums/topic/29236?comment=262386#262386
image

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. This proved exceptionally difficult to debug and fix.

The root cause of the issue is the first spoiler tag in the post, which contains placeholders.

I discovered the following input worked great:

content here

[spoiler]
[ach=9]
[game=14492]
[url=https://google.com]My Link[/url]
[/spoiler]

However, this input made things go off the rails:

content here

[spoiler]
[ach=]
[url=]Link[/url]
[/spoiler]

[spoiler]
[ach=9]
[game=14492]
[url=https://google.com]My Link[/url]
[/spoiler]

The 3rd party <BBCode /> component seems overly greedy towards the "placeholder" tags. Despite my efforts, I couldn't get it to ignore trying to process them.

To work around the issue, I made some changes to the postprocessor to trick <BBCode /> into not processing these placeholders. When the postprocessor sees a placeholder like [ach=], it now converts it to [text]{ach=}[/text]. A new <ShortcodeText /> component then converts the content back to plain text: "[ach=]".

This is a lot of arm wrestling to get things to work, so I made sure it's well-covered by tests.

<>
<SEO title={forumTopic.title as TranslatedString} description={description} />

<AppLayout.Main>
Copy link
Member

Choose a reason for hiding this comment

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

Breadcrumb is escaping foreign characters:
image

Copy link
Member Author

Choose a reason for hiding this comment

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

Surprisingly, it appears this is how the forum title is literally stored in the database (check the forums table). We should probably manually update the title to "RA em Português!".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants