Laushinka/error form 10062 #10071

laushinka · 2022-05-17T17:15:21Z

Description

Shows feedback component when there are error messages on:

login
create workspace

MINIMISED	EXPANDED	THANKS

Related Issue(s)

Fixes #10062

How to test

(A bit tedious to make sure to get the error message, sorry)

Go to your user in the /admin page and remove the repo feature that gives permission for private repos.
Start a workspace for that private repo, i.e. https://laushinka-f9a883104c.preview.gitpod-dev.com/#[YOUR_PRIVATE_REPO]
See the feedback form below the error message.

Release Notes

Feedback form under error messages on login and starting workspaces.

Documentation

/werft analytics=segment|TEZnsG4QbLSxLfHfNieLYGF4cDwyFWoe

jldec · 2022-05-23T19:30:00Z

Thanks @laushinka - that works well - I have a few layout suggestions mainly intended to tone down the new UX by making it smaller and lighter and a bit more separate.

minimized - use smaller text (matching grey above) and separate with a border top and margin top - (the feedback widget should be somewhat separate, and not distract from the error)
expanded - use grey background around form (maybe rounded corners) instead of horizontal rules
thanks - same smaller grey text

MINIMISED	EXPANDED	THANKS

laushinka · 2022-05-24T09:09:58Z

Thanks @laushinka - that works well - I have a few layout suggestions

@jldec I thought I was editing the images in the description but I ended up editing your images 🥲

laushinka · 2022-05-24T09:16:28Z

@gtsiolis I had to remove the fixed height here from the previous PR [1] because I couldn't make it work with Tailwind classnames for some reason 🤔

laushinka · 2022-05-24T09:25:48Z

@jldec Ready for another review.

laushinka · 2022-05-24T09:48:27Z

Rerunning werft for the analytics flag.

/werft run

👍 started the job as gitpod-build-laushinka-error-form-10062.17
(with .werft/ from main)

gtsiolis

Thanks @laushinka for pushing this forward!

Sorry for hijacking the review, but this PR grabbed my attention from UX perspective.

Please, let me know if there's a better way to send feedback like the following and be respectful of everyone's time, avoid reverting our own UX changes, and avoiding back and forth in development. If needed, let's move all the UX feedback below to a separate follow-up issue. ➿ Cc @jldec

components/dashboard/src/feedback-form/FeedbackModal.tsx

components/dashboard/src/feedback-form/FeedbackComponent.tsx

gtsiolis · 2022-05-24T09:40:08Z

components/dashboard/src/feedback-form/FeedbackComponent.tsx

+            {minimisedFirstView && (
+                <div
+                    className={
+                        "flex flex-col justify-center px-6 border-t border-gray-200 dark:border-gray-800 " +


question: Did we discuss anywhere the idea of reusing the existing feedback modal when a user clicks on one of the emojis? This could reuse the same component as is and save us a ton of vertical space, which is quite crucial in the workspace start page.

Do you mean that instead of expanding in page, we would trigger the modal? Good point re the vertical space, but in an error page, the user might want to keep the error and context always in sight, instead of blocked by a modal.
Let me also loop in @jldec

Do you mean that instead of expanding in page, we would trigger the modal?

Yes.

Good point re the vertical space, but in an error page, the user might want to keep the error and context always in sight, instead of blocked by a modal.

Fair point, just wanted to bring this up in case this has not been taken into account and could be worth considering.

components/dashboard/src/feedback-form/FeedbackComponent.tsx

gtsiolis · 2022-05-24T09:46:49Z

components/dashboard/src/feedback-form/FeedbackComponent.tsx


-    const height = props.isModal ? "300px" : "";


question: Coming to this line triggered from the comment in #10071 (comment). Although non-blocking and a minor issue, is there a technical limitation that led us to remove this?

~~Made a comment here~~ Ah you already linked it. Yes that was the technical limitation from my end - would love some pointers here!

@laushinka Haven't checked this in-depth, but I was thinking we could explicitly and conditionally set padding using utility classes for having a fixed modal height in the case of the modal, as we do now in https://github.com/gitpod-io/gitpod/pull/10071/files#diff-51811862e7996f416f9f7d73ff24341eecd96f3c083fb46d1b00b6cf94b50686R139. Since we're using a specific typography and spacing scale system, there must be a padding values that could help us set a modal height exactly as it the step 2 of the feedback modal.

gtsiolis

@laushinka UX review round two, done.

I have a feeling that our attempt to reuse some components in combination with the limited discussion of the UX of the scope of this change has led us to go into small circles. If you think any UX feedback here can be addressed separately, let's do that, as the initial state of this PR carried quite a few UX issues and don't want to block this from being merged. Just bringing this up for visibility. Cc @jldec

gtsiolis · 2022-05-24T14:44:02Z

components/dashboard/src/feedback-form/FeedbackComponent.tsx

+                        <div className="flex justify-end mt-6">
+                            <button className="secondary" onClick={onClose}>
+                                Cancel
+                            </button>
+                            <button className="ml-2" onClick={onSubmit}>
+                                Send Feedback
+                            </button>
+                        </div>


issue: Just realized you're trying to reuse the same component in both cases, however, this is breaking the modal structure (see line separator below action buttons) and the padding bug now has moved to the first step of the feedback step.

Feedback Modal (Step 1) Feedback Modal (Step 2)

gtsiolis · 2022-05-24T14:46:04Z

components/dashboard/src/feedback-form/FeedbackComponent.tsx

-                    <p className="text-center text-lg mb-8 text-gray-500 dark:text-gray-400">
-                        We'd love to know what you think!
+                    <p className={"text-center text-base " + (props.isError ? "text-gray-400" : "text-gray-500")}>
+                        Thanks for your feedback, we appreciate it.


praise: Thanks for also including this step in the feedback modal used in the dashboard pages! 🙏

gtsiolis · 2022-05-24T14:47:20Z

components/dashboard/src/feedback-form/FeedbackComponent.tsx

-                    <p className="text-center text-lg mb-8 text-gray-500 dark:text-gray-400">
-                        We'd love to know what you think!
+                    <p className={"text-center text-base " + (props.isError ? "text-gray-400" : "text-gray-500")}>
+                        Thanks for your feedback, we appreciate it.


suggestion: Since this PR now also includes the last step of the feedback widget, we could also align the designs to include the 🙏 emoji? You can grab the SVG from the specs included in #10124. Going with a simple inline structure sounds good. WDYT?

Closable last step Non-closable last step

In case this sounds interesting to you, we could also reuse the alert component for this which now supports opting in for closable option and an icon, but let's skip this for now.

gtsiolis · 2022-05-24T17:47:10Z

components/dashboard/src/feedback-form/FeedbackComponent.tsx

-                    <p className="text-center text-lg mb-8 text-gray-500 dark:text-gray-400">
-                        We'd love to know what you think!
+                    <p className={"text-center text-base " + (props.isError ? "text-gray-400" : "text-gray-500")}>
+                        Thanks for your feedback, we appreciate it.


issue: Removing the modal header could confuse users and introduces an inconsistent behavior of the feedback action. Maybe we could skip the header altogether moving away from the modal component as we want to introduce so many customizations here.

Thanks (Modal) Thanks (Error)

gtsiolis · 2022-05-24T17:47:47Z

components/dashboard/src/feedback-form/FeedbackComponent.tsx

                >
-                    <p className="text-center text-lg mb-8 text-gray-500 dark:text-gray-400">
-                        We'd love to know what you think!
+                    <p className={"text-center text-base " + (props.isError ? "text-gray-400" : "text-gray-500")}>


suggestion: In both cases, being able to dismiss the thanks message at the last step would be great to keep.

gtsiolis · 2022-05-24T17:58:13Z

components/dashboard/src/feedback-form/FeedbackComponent.tsx

-                    <div className="flex flex-col -mx-6 px-6 py-4 border-t border-b border-gray-200 dark:border-gray-800 bg-white dark:bg-gray-900">
+                    <div
+                        className={
+                            "flex flex-col px-6 py-4 border-gray-200 dark:border-gray-800 bg-white dark:bg-gray-900 " +


suggestion: Minor color background and padding fix.

Suggested change

"flex flex-col px-6 py-4 border-gray-200 dark:border-gray-800 bg-white dark:bg-gray-900 " +

"flex flex-col px-4 py-4 border-gray-200 dark:border-gray-800 bg-white dark:bg-gray-800 " +

gtsiolis · 2022-05-24T17:58:15Z

components/dashboard/src/feedback-form/FeedbackComponent.tsx

+                                ? "w-96 mt-6 bg-gray-100 dark:bg-gray-800 rounded-xl"
+                                : "-mx-6 border-t border-b")
+                        }
+                    >
                        <div className="relative">
                            <div className="absolute flex bottom-5 right-5 -space-x-3">{emojiGroup(24)}</div>
                            <textarea


suggestion: Some class changes to restore some color changes as I cannot add this code suggestion directly on the line.

For https://github.com/gitpod-io/gitpod/pull/10071/files#diff-51811862e7996f416f9f7d73ff24341eecd96f3c083fb46d1b00b6cf94b50686R109:

w-full resize-none text-gray-600 dark:text-gray-300 focus:ring-0 focus:border-gray-400 dark:focus:border-gray-400 rounded-md border dark:bg-gray-800 dark:border-gray-500 border-gray-300

gtsiolis · 2022-05-24T18:03:04Z

components/dashboard/src/feedback-form/FeedbackComponent.tsx

+                        className={
+                            "flex flex-col px-6 py-4 border-gray-200 dark:border-gray-800 bg-white dark:bg-gray-900 " +
+                            (props.isError
+                                ? "w-96 mt-6 bg-gray-100 dark:bg-gray-800 rounded-xl"


suggestion: Minor bottom margin to always have some whitespace around the modal.

Suggested change

? "w-96 mt-6 bg-gray-100 dark:bg-gray-800 rounded-xl"

? "w-96 mt-6 bg-gray-50 dark:bg-gray-800 rounded-xl mb-4"

gtsiolis · 2022-05-24T18:14:01Z

components/dashboard/src/feedback-form/FeedbackComponent.tsx

@@ -82,26 +123,27 @@ function FeedbackComponent(props: { onClose: () => void; onSubmit: () => void; i
                                .
                            </p>
                        </div>
-                    </div>


Restoring text alignment from #10071 (comment).

Suggestion for https://github.com/gitpod-io/gitpod/pull/10071/files#diff-51811862e7996f416f9f7d73ff24341eecd96f3c083fb46d1b00b6cf94b50686R117:

text-gray-500 text-left

laushinka · 2022-05-25T08:43:48Z

@laushinka If you think any UX feedback here can be addressed separately, let's do that

@gtsiolis Thanks, George! Definitely agree that we can carry over the UI feedback in a follow-up issue in case not all are tackled here. Appreciate the detailed look into the design discrepancies 🙏🏽

andrew-farries · 2022-05-25T09:07:45Z

components/dashboard/src/feedback-form/FeedbackComponent.tsx

+        if (props.onClose) {
+            props.onClose();
+        }


Can you use optional chaining here?

Thanks for the review, @andrew-farries! I tried that but the linter still didn't like it 🙈

roboquat added do-not-merge/work-in-progress do-not-merge/release-note-label-needed size/XL labels May 17, 2022

laushinka force-pushed the laushinka/error-form-10062 branch from a649215 to e6fe246 Compare May 20, 2022 07:45

roboquat added size/L and removed size/XL labels May 20, 2022

laushinka force-pushed the laushinka/error-form-10062 branch 3 times, most recently from 79bcd31 to ab9830a Compare May 23, 2022 14:58

roboquat added release-note and removed do-not-merge/release-note-label-needed labels May 23, 2022

laushinka requested a review from jldec May 23, 2022 15:05

laushinka force-pushed the laushinka/error-form-10062 branch 3 times, most recently from 3987aeb to 469cc5c Compare May 23, 2022 18:35

laushinka force-pushed the laushinka/error-form-10062 branch from 469cc5c to 196c5e7 Compare May 23, 2022 21:07

laushinka force-pushed the laushinka/error-form-10062 branch from 196c5e7 to 2b4824d Compare May 24, 2022 09:14

laushinka marked this pull request as ready for review May 24, 2022 09:15

laushinka requested a review from a team May 24, 2022 09:15

roboquat removed the do-not-merge/work-in-progress label May 24, 2022

github-actions bot added the team: webapp Issue belongs to the WebApp team label May 24, 2022

gtsiolis reviewed May 24, 2022

View reviewed changes

Feedback modal

50b79c0

laushinka force-pushed the laushinka/error-form-10062 branch from 882aa12 to 50b79c0 Compare May 24, 2022 13:50

gtsiolis reviewed May 24, 2022

View reviewed changes

andrew-farries approved these changes May 25, 2022

View reviewed changes

roboquat merged commit c841bd2 into main May 25, 2022

roboquat deleted the laushinka/error-form-10062 branch May 25, 2022 09:08

gtsiolis mentioned this pull request May 27, 2022

Follow-up from adding the feedback widget on error pages #10307

Open

9 tasks

roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels May 31, 2022

This was referenced Jun 1, 2022

Add feedback successful submission step in the feedback modal #10124

Closed

[analytics] add error field to feedback event #10455

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Laushinka/error form 10062 #10071

Laushinka/error form 10062 #10071

laushinka commented May 17, 2022 •

edited

Loading

jldec commented May 23, 2022 •

edited by laushinka

Loading

laushinka commented May 24, 2022 •

edited by werft-gitpod-dev-com bot

Loading

laushinka commented May 24, 2022 •

edited by werft-gitpod-dev-com bot

Loading

laushinka commented May 24, 2022

laushinka commented May 24, 2022 •

edited by werft-gitpod-dev-com bot

Loading

gtsiolis left a comment •

edited

Loading

gtsiolis May 24, 2022

laushinka May 24, 2022 •

edited

Loading

gtsiolis May 24, 2022 •

edited

Loading

gtsiolis May 24, 2022

laushinka May 24, 2022 •

edited

Loading

gtsiolis May 24, 2022 •

edited

Loading

gtsiolis left a comment •

edited

Loading

gtsiolis May 24, 2022

gtsiolis May 24, 2022

gtsiolis May 24, 2022 •

edited

Loading

gtsiolis May 24, 2022

gtsiolis May 24, 2022

gtsiolis May 24, 2022

gtsiolis May 24, 2022

gtsiolis May 24, 2022

gtsiolis May 24, 2022

laushinka commented May 25, 2022 •

edited by werft-gitpod-dev-com bot

Loading

andrew-farries May 25, 2022

laushinka May 25, 2022

	"flex flex-col px-6 py-4 border-gray-200 dark:border-gray-800 bg-white dark:bg-gray-900 " +
	"flex flex-col px-4 py-4 border-gray-200 dark:border-gray-800 bg-white dark:bg-gray-800 " +

	? "w-96 mt-6 bg-gray-100 dark:bg-gray-800 rounded-xl"
	? "w-96 mt-6 bg-gray-50 dark:bg-gray-800 rounded-xl mb-4"

Laushinka/error form 10062 #10071

Laushinka/error form 10062 #10071

Conversation

laushinka commented May 17, 2022 • edited Loading

Description

Related Issue(s)

How to test

Release Notes

Documentation

jldec commented May 23, 2022 • edited by laushinka Loading

laushinka commented May 24, 2022 • edited by werft-gitpod-dev-com bot Loading

laushinka commented May 24, 2022 • edited by werft-gitpod-dev-com bot Loading

laushinka commented May 24, 2022

laushinka commented May 24, 2022 • edited by werft-gitpod-dev-com bot Loading

gtsiolis left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laushinka May 24, 2022 • edited Loading

Choose a reason for hiding this comment

gtsiolis May 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laushinka May 24, 2022 • edited Loading

Choose a reason for hiding this comment

gtsiolis May 24, 2022 • edited Loading

Choose a reason for hiding this comment

gtsiolis left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gtsiolis May 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laushinka commented May 25, 2022 • edited by werft-gitpod-dev-com bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laushinka commented May 17, 2022 •

edited

Loading

jldec commented May 23, 2022 •

edited by laushinka

Loading

laushinka commented May 24, 2022 •

edited by werft-gitpod-dev-com bot

Loading

laushinka commented May 24, 2022 •

edited by werft-gitpod-dev-com bot

Loading

laushinka commented May 24, 2022 •

edited by werft-gitpod-dev-com bot

Loading

gtsiolis left a comment •

edited

Loading

laushinka May 24, 2022 •

edited

Loading

gtsiolis May 24, 2022 •

edited

Loading

laushinka May 24, 2022 •

edited

Loading

gtsiolis May 24, 2022 •

edited

Loading

gtsiolis left a comment •

edited

Loading

gtsiolis May 24, 2022 •

edited

Loading

laushinka commented May 25, 2022 •

edited by werft-gitpod-dev-com bot

Loading