-
-
Notifications
You must be signed in to change notification settings - Fork 99
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: migrate forum post Edit page to React #3021
feat: migrate forum post Edit page to React #3021
Conversation
patches/@bbob__preset-react.patch
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both of these .patch files are a pnpm thing to ensure the React shortcode dependency doesn't break SSR. They're both referenced in package.json and both need to be committed.
"@floating-ui/core": "^1.6.8", | ||
"@floating-ui/dom": "^1.5.1", | ||
"@hookform/resolvers": "^3.9.0", | ||
"@inertiajs/core": "^1.2.0", | ||
"@radix-ui/react-alert-dialog": "^1.1.4", | ||
"@radix-ui/react-checkbox": "^1.1.1", | ||
"@radix-ui/react-collapsible": "^1.1.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This powers a base component for the "Spoiler" button, but can also be used for other collapses, such as the activity cards on the new activity page.
@@ -31,6 +31,7 @@ class Ticket extends BaseModel | |||
// TODO rename Updated column to updated_at | |||
// TODO drop AchievementID, use ticketable morph instead | |||
// TODO drop Hardcore, derived from player_session | |||
// TODO rename ticketable_model to ticketable_type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ticketable_model
implies we'll be storing a fully-qualified model name to the database, ie: "App\\Models\\Achievement"
.
I think ticketable_type
is a more desirable field, as it can just be an enum.
public int $id, | ||
public TicketableType $ticketableType, | ||
public Lazy|int $state, | ||
public Lazy|AchievementData|LeaderboardData|GameData $ticketable, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small prep here in this new Ticket
DTO for multiple ticketable types. This is just future-thinking ... everything in fromTicket()
hardcodes to TicketableType
being an achievement.
export const ShortcodeRenderer: FC<ShortcodeRendererProps> = ({ body }) => { | ||
return ( | ||
<BBCode | ||
container="div" | ||
plugins={plugins} | ||
options={{ | ||
onlyAllowTags: [ | ||
'b', | ||
'i', | ||
'u', | ||
's', | ||
'code', | ||
'spoiler', | ||
'img', | ||
'url', | ||
'user', | ||
'ach', | ||
'game', | ||
'ticket', | ||
'video', | ||
], | ||
}} | ||
> | ||
{postProcessShortcodesInBody(body)} | ||
</BBCode> | ||
); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the component that takes an input containing shortcodes and actually parses+renders them to the screen.
In theory, we could also use this in comment sections with a smaller subset of onlyAllowTags
values. Maybe this could be controlled as a prop?
// Twitch video regex pattern: | ||
// (?:https?://)? # Optional scheme. Either http or https. | ||
// (?:www.)? # Optional subdomain. | ||
// twitch.tv/.* # Host. | ||
// (?:videos|[^/]+/v) # Match either "videos" or "{username}/v" | ||
// /(\d+) # Video ID (numeric) | ||
// (?! # Assert URL is not pre-linked. | ||
// [?=&+%\w.-]* # Allow URL (query) remainder. | ||
// (?: # Group pre-linked alternatives. | ||
// [^<>]*> # Either inside a start tag, | ||
// | [^<>]*</a> # or inside <a> element text contents. | ||
// ) # End recognized pre-linked alts. | ||
// ) # End negative lookahead assertion. | ||
// ([?=&+%\w.-]*) # Consume any URL (query) remainder. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is ported directly from our PHP implementation.
// This regex pattern matches YouTube URLs with the following structure: | ||
// (?:https?://)? # Optional scheme. Either http or https. | ||
// (?:[0-9A-Z-]+\.)? # Optional subdomain. | ||
// (?: # Group host alternatives. | ||
// youtu\.be/ # Either youtu.be (trailing slash required), | ||
// | youtube\.com # or youtube.com followed by | ||
// \S* # Allow anything up to VIDEO_ID, | ||
// [^\w\\-\s] # but char before ID is non-ID char. | ||
// ) # End host alternatives. | ||
// ([\w\-]{11}) # $1: VIDEO_ID is exactly 11 chars. | ||
// (?=[^\w\-]|$) # Assert next char is non-ID or EOS. | ||
// (?! # Assert URL is not pre-linked. | ||
// (?: # Group pre-linked alternatives. | ||
// [^<>]*> # Either inside a start tag, | ||
// | [^<>]*</a> # or inside <a> element text contents. | ||
// ) # End recognized pre-linked alts. | ||
// ) # End negative lookahead assertion. | ||
// ([?=&+%\w.-]*) # Consume any URL (query) remainder. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is ported directly from our PHP implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went through my last twenty posts or so and did edit+preview and compared to edit2+preview.
Also created a new post, and used all the buttons to modify stuff.
Found a couple things. Overall it seems to work well. Have not looked at the code changes.
resources/js/features/forums/components/EditPostMainRoot/EditPostForm/EditPostForm.tsx
Outdated
Show resolved
Hide resolved
resources/js/features/forums/components/ShortcodeRenderer/ShortcodeRenderer.test.tsx
Show resolved
Hide resolved
resources/js/features/forums/components/ShortcodeRenderer/ShortcodeRenderer.test.tsx
Show resolved
Hide resolved
I'm going to merge this and replace the PHP |
Terribly sorry for the huge PR. This turned out to be way more work than I thought it was going to be.
This PR begins the process of migrating the forum post edit page to React. Similar to the activity page migration, both the PHP UI and React UI co-exist on this branch so you can compare the two.
How to test this PR:
Navigate to both of these URLs:
http://localhost:64000/forums/post/181237/edit
http://localhost:64000/forums/post/181237/edit2
Compare and contrast.
The form itself was trivial to port. The preview button, on the other hand, was extremely complex - mostly due to the need to completely rewrite the logic for rendering our shortcodes. While porting our shortcode functionality to React, I identified and fixed several bugs in our current shortcode implementation (fixes are only in the React version). Most of the bugs in our shortcode implementation have to do with self-closing
[url]
shortcode tags (ie:[url=https://google.com]
).A major difference in how the preview functionality in the React UI works is we obviously can't just render a React component tree in the Laravel back-end and send it to the front-end. The front-end itself has to do the work of rendering the preview. Here's the logic:
Aside from a few bug fixes (which you'll see with the test inputs below if you compare them between
/edit
and/edit2
), there are very few minor rendering changes:All existing embed functionality is preserved. Here are a few good test inputs to compare the React UI and PHP UI:
Brave.Browser.mp4