-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
fix(replays): add prompt if user needs SDK upgrade #53672
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
Conversation
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.
I'm wondering if this is a bit too subtle? Thoughts on using an Alert banner with the message?
We should also link to something as well... maybe https://docs.sentry.io/platforms/javascript/install/npm/? We don't really have any upgrading docs unfortunately.
static/app/views/replays/list/replaysErroneousDeadRageCards.tsx
Outdated
Show resolved
Hide resolved
${props => | ||
props.gridRows | ||
? `grid-template-rows: ${props.gridRows};` | ||
: 'grid-template-rows: 44px max-content'} |
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.
@ryan953 shhhh
border-radius: 0px; | ||
border-width: 1px 0 1px 0; |
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.
the styled alert looks better inside the cards 👍
can you also check how it looks inside the main table? that might not have a min-height but idk how the extra border will be.
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.
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.
do we want to remove the top border for both alerts here?
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.
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.
issue stream also has that margin. i don't love the margin though
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.
i can add in that margin to the style for continuity
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.
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.
discussed offline: @ryan953 suggested keeping rage click table alert as is, but removing margin & bottom border for big replay table
3a70dea
to
350905f
Compare
@@ -28,6 +31,8 @@ import { | |||
import {ReplayColumn} from 'sentry/views/replays/replayTable/types'; | |||
import type {ReplayListRecord} from 'sentry/views/replays/types'; | |||
|
|||
export const MIN_DEAD_RAGE_CLICK_SDK = '7.60.1'; |
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.
Is this imported somewhere?
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.
was planning to originally but not anymore -- i'll get rid of it
350905f
to
0486d56
Compare
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.
It's harder without designs, but I think we saw screen shots of different options and the result looks good!
fixes getsentry/team-replay#126 by informing users they need to upgrade their SDK to see rage/dead click data if this applies.
no results but SDK version is good:

no results and SDK version too low:

error alert also updated:

