Add feedback modal #857
Add feedback modal #857
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
I'd perhaps add some note that when sending the form we won't include any details about the project, user's setup or the logs |
|
Building on what @kmagiera has commented, maybe we could add an "opt-in" check box for attaching radon logs? |
|
getting the logs is not as straightforward, also this would fit better in bug/issue reporting flow rather than feedback, so I'd leave it out. Just wanted to make sure it is clear the feedback form doesn't include any potentially sensitive info |
Those paddings look odd. |
|
@maciekstosio fixed in 0947f14 |
|
alternatively i can gather the feedback in our customer portal database |
|
I guess this should be fine for now. One simple workaround we can add is to check if user have telemetry on and only then show this button |
…tate to the top component to allow better state menagment
| return <UtilsContext.Provider value={utils}>{children}</UtilsContext.Provider>; | ||
| const [telemetryEnabled, setTelemetryEnabled] = useState(false); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
I think it should be in a separate hook, as a change to the state will cause unnecessary rerenders in all useUtils occurrences
There was a problem hiding this comment.
Something like this will be better imo.
const useTelemntry = () => {
const utils = useUtils();
const [telemetryEnabled, setTelemetryEnabled] = useState(false);
useEffect(() => {
utils.isTelemetryEnabled().then(setTelemetryEnabled);
utils.addListener("telemetryEnabledChanged", setTelemetryEnabled);
return () => {
utils.removeListener("telemetryEnabledChanged", setTelemetryEnabled);
};
}, []);
return telemetryEnabled;
}
| return <UtilsContext.Provider value={utils}>{children}</UtilsContext.Provider>; | ||
| const [telemetryEnabled, setTelemetryEnabled] = useState(false); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Something like this will be better imo.
const useTelemntry = () => {
const utils = useUtils();
const [telemetryEnabled, setTelemetryEnabled] = useState(false);
useEffect(() => {
utils.isTelemetryEnabled().then(setTelemetryEnabled);
utils.addListener("telemetryEnabledChanged", setTelemetryEnabled);
return () => {
utils.removeListener("telemetryEnabledChanged", setTelemetryEnabled);
};
}, []);
return telemetryEnabled;
}
| const utils = makeProxy<Utils>("Utils"); | ||
|
|
||
| const UtilsContext = createContext<UtilsInterface>(utils); | ||
| const UtilsContext = createContext<UtilsContextProps>(utils); |
| }, []); | ||
|
|
||
| return ( | ||
| <TelemetryContext.Provider value={{ telemetryEnabled }}>{children}</TelemetryContext.Provider> |
There was a problem hiding this comment.
{telemetryEnabled} is useMemo -> if the value doesn't change, we'll rerender descend components anyway, cause the object will be recreated.
| const context = useContext(TelemetryContext); | ||
|
|
||
| if (context === undefined) { | ||
| throw new Error("useUtils must be used within a TelemetryContextProvider"); |
There was a problem hiding this comment.
| throw new Error("useUtils must be used within a TelemetryContextProvider"); | |
| throw new Error("useTelemetry must be used within a TelemetryContextProvider"); |
| }; | ||
|
|
||
| export function Feedback({ sentiment, setSentiment }: FeedbackProps) { | ||
| const { sendTelemetry } = useUtils(); |
There was a problem hiding this comment.
If we have a separate telemetry hook, it sounds like sendTelemetry should be there
There was a problem hiding this comment.
it will be on Utils interface anyway since i have no means of removing it, so it seems pointless to add it a second tim in useTelemetry
|
|
||
| const TelemetryContext = createContext<TelemetryContextProps>({ telemetryEnabled: false }); | ||
|
|
||
| export function TelemetryProvider({ children }: PropsWithChildren) { |
There was a problem hiding this comment.
IMO, it should be in a separate file. I am not sure why we need to pass utils through the context, but if we cannot just import it in a separate file, you can always use useUtils in TelemetryProvider. Just remember to order providers properly (imo this is why it would be better to have it just in hook instead of context, especially since we don't use it anywhere else).
| }, []); | ||
|
|
||
| return ( | ||
| <TelemetryContext.Provider value={{ telemetryEnabled }}>{children}</TelemetryContext.Provider> |
There was a problem hiding this comment.
return just telemetryEnabled or wrap with useMemo
This PR implements feedback functionality by gathering
sentiment:positive,sentiment:negativetelemetry events.Screen.Recording.2024-12-16.at.16.15.25.mov
Screen.Recording.2024-12-16.at.16.15.49.mov