-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
chore: Refactor FeedbackButton to accept all the feedback options #103794
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
995a808 to
5f5f8bf
Compare
Also I have made some extra layers and older systems deprecated. I will try to followup and remove them fully afterwards
5f5f8bf to
9a189b1
Compare
|
@sentry review |
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 was moved into feedbackButton/floatingFeedbackButton.tsx and got parts of useFeedbackWidget inserted into it along with a big docstring, which means git isn't tracking it as a move anymore.
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 was moved into feedbackButton/feedbackButton.tsx and got parts of useFeedbackWidget inserted into it along with a big docstring, which means git isn't tracking it as a move anymore.
| /** | ||
| * @deprecated This layer isn't needed. Call `useFeedbackSDKIntegration` or use `<FeedbackButton/>` or `<FloatingFeedbackButton/>` | ||
| */ | ||
| export default function useFeedbackWidget({buttonRef}: Props) { |
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 deprecated now. it's a layer that isn't providing a lot of value.
I've already inlined bits into FeedbackButton and FloatingFeedbackButton,
Next step is cleanup: there's one more callsite to go and remove.
| /** | ||
| * @deprecated Use components/feedbackButton/feedbackButton.tsx instead. | ||
| */ | ||
| export default function FeedbackButton() { |
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 name is tooo generic. Wrong FeedbackButton if you found it.
It renders the old-style FeedbackModal in self-serve, or a Zendesk button in SaaS. I plan to replace it with the regular components/feedbackButton/feedbackButton, or we can take it all away too, tbd.
static/app/components/feedbackButton/useFeedbackSDKIntegration.tsx
Outdated
Show resolved
Hide resolved
| * | ||
| * @deprecated This hook is too low level. Use `<FeedbackButton/>` or `<FloatingFeedbackButton/>` instead. | ||
| */ | ||
| export function useFeedbackForm() { |
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 hook is doing a lot of what the feedback sdk does already.
This has some comparison logic so that if optionOverrides is deep-equal to the prev value passed we'll re-use the existing form (it's a deep equal compare, not ref compare). For end-users this means their half-written message will pop back open if they re-click the same "Give Feedback" button, or if there are two "Give Feedback" buttons with the same overrides. Clicking a third "Give Feedback" button will clear out the half-written message and show a new form.
Since tags are commonly passed in all the time, this behavior really only impacts places where we're rendering the button in a loop, otherwise options will not be the same between two instances.
We can get very similar behavior if we use feedback.attachTo() instead of feedback.createForm, it'll re-use the existing form if you click the same button twice in a row.
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.
Turns out this hook is also useful for times when the Button is inside a Dropdown or similar. The button might be unmounted but we don't want the feedback dialog to also unmount. The feedback dialog should be opened but not tethered to the lifecycle of the button that opened it.
…erms of useFeedbackForm
There's a rename in here which is why a bunch of extra files got touched. Also I have made some extra layers and older systems deprecated. I will try to followup and remove them fully afterwards