Skip to content

fix(replays): Correctly check for session-replay feature before deciding whether to fetch or show replay onboarding#45871

Merged
ryan953 merged 3 commits into
masterfrom
ryan953/9821-fix-replay-onboarding-conditions-order
Mar 16, 2023
Merged

fix(replays): Correctly check for session-replay feature before deciding whether to fetch or show replay onboarding#45871
ryan953 merged 3 commits into
masterfrom
ryan953/9821-fix-replay-onboarding-conditions-order

Conversation

@ryan953

@ryan953 ryan953 commented Mar 15, 2023

Copy link
Copy Markdown
Member

…ing whether to fetch or show replay onboarding
@ryan953 ryan953 requested a review from a team as a code owner March 15, 2023 20:44
@ryan953 ryan953 requested a review from a team March 15, 2023 20:44
@github-actions github-actions Bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Mar 15, 2023
isEmpty={replays?.length === 0}
isLoading={isFetching}
visibleColumns={visibleColumns}
data-test-id="replay-table"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why the mix of replay-table and replayTable?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I started with replayTable then switched to kebab because it seems like the convention.

fixing.

});

return (
<Page>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: you have ReplaysListFetcher wrapping ReplayOnboardingPanel with Page, but have Page here inside of ReplayListTable.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

i think now that ReplaysListFetcher isn't doing much.

I tore it apart because the useReplayList was happening unconditionally, it's in too many bits right now i think

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

woah! so much better now. <Page> is no longer a thing ,but there's no duplicate of that stuff.

And just one inner component inside this file.

@ryan953 ryan953 force-pushed the ryan953/9821-fix-replay-onboarding-conditions-order branch from a23d0df to 5c51941 Compare March 15, 2023 21:22

@billyvg billyvg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ryan953 ryan953 merged commit 6109030 into master Mar 16, 2023
@ryan953 ryan953 deleted the ryan953/9821-fix-replay-onboarding-conditions-order branch March 16, 2023 15:18
@github-actions github-actions Bot locked and limited conversation to collaborators Apr 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants