We create a custom MarkdownSpoiler component to render spoiler content in-line, but to add additional functionality within the component in order to not dilute rules.react.js.
Depends on D5546
Details
Details
The expected behavior is the same as the previous diff - the spoiler text still renders in a span.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- spoiler_web
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| web/markdown/markdown-spoiler.react.js | ||
|---|---|---|
| 15 ↗ | (On Diff #18184) | My reasoning for containing the Spoiler text in React.useMemo() is I believe there's a linear task to track editing / deleting messages, so ultimately when that makes progress it's possible that the text prop will change and we can trigger the dependency change to not get the cached value, but for all other use cases we don't need to keep re-rendering the same spoiler text when the text content is not changing. |
Comment Actions
Can we remove the children prop? Doesn't look like it's being used in this diff
| web/markdown/markdown-spoiler.react.js | ||
|---|---|---|
| 9 ↗ | (On Diff #18184) | It doesn't look like we're using this children prop? |
| web/markdown/markdown-spoiler.react.js | ||
|---|---|---|
| 14–16 | Instead of memoizing the return value you should just wrap the whole component in React.memo | |
| web/markdown/markdown-spoiler.react.js | ||
|---|---|---|
| 11–17 ↗ | (On Diff #18332) | Let's avoid the increased indentation level here |
| web/markdown/markdown-spoiler.react.js | ||
|---|---|---|
| 11–17 ↗ | (On Diff #18332) | Thanks for the feedback - do you mind clarifying what you mean exactly about the indentation? |
| web/markdown/markdown-spoiler.react.js | ||
|---|---|---|
| 11–17 ↗ | (On Diff #18332) | Something like this example in the codebase:  |
| web/markdown/markdown-spoiler.react.js | ||
|---|---|---|
| 11–17 ↗ | (On Diff #18332) | Ah makes sense, thanks for the reference! |