Convert beatmap discussions main component to typescript #10409
Conversation
notbakaneko
force-pushed
the
feature/ts-discussions-main
branch
3 times, most recently
from
f83df57
to
f222821
Compare
notbakaneko
force-pushed
the
feature/ts-discussions-main
branch
from
9e34a2e
to
2d7bc76
Compare
...maybe explicit key is better? 🤔
| function isBeatmapsetDiscussionJsonForShow(value: Props['discussion']): value is BeatmapsetDiscussionJsonForShow { | ||
| return 'posts' in value; | ||
| } |
There was a problem hiding this comment.
inlining 'posts' in discussion also works? 🤔 doesn't need manual type predicates
There was a problem hiding this comment.
The parts that use it are more "render if component is on the page that uses this type (so just main discussions page)" rather than "render if this property is available", it's just coincidental that posts is currently enough to discriminate the type.
There was a problem hiding this comment.
I thought it's handled by the readonly props?
| } | ||
|
|
||
| toJson() { | ||
| return { |
There was a problem hiding this comment.
this should have a note about non-trivial values needing matching reviver especially since there won't be any typing involved
| ? this.props.currentDiscussions.countsByBeatmap[beatmap.id] | ||
| : undefined; | ||
| // TODO: does it need to be computed? | ||
| private readonly getCount = (beatmap: BeatmapExtendedJson) => beatmap.deleted_at == null ? this.discussionsState.discussionsByBeatmap(beatmap.id).length : undefined; |
There was a problem hiding this comment.
the one here should be unresolved count, not total
| entries={gameModes.map((mode) => ({ | ||
| count: this.props.currentDiscussions.countsByPlaymode[mode], | ||
| disabled: (this.props.beatmaps.get(mode)?.length ?? 0) === 0, | ||
| count: this.discussionsState.discussionsCountByPlaymode[mode], |
| document.documentElement.style.removeProperty('--scroll-padding-top-extra'); | ||
|
|
||
| window.clearTimeout(this.timeoutCheckNew); | ||
| this.xhrCheckNew?.abort(); | ||
| this.disposers.forEach((disposer) => disposer?.()); | ||
| this.discussionsState.destroy(); |
There was a problem hiding this comment.
I think these should also be run on turbolinks:before-cache. the destroy() notably involves removing url handler which means if the state somehow changes between new page load and unmount, the url will be wrong
Includes refactoring the discussion state handling out of the top-level component into a separate object
Draft while I double check stuff since it's pretty large from moving all the state handling out of the component.