Laushinka/error form 10062 #10071
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| /** | ||
| * Copyright (c) 2022 Gitpod GmbH. All rights reserved. | ||
| * Licensed under the GNU Affero General Public License (AGPL). | ||
| * See License-AGPL.txt in the project root for license information. | ||
| */ | ||
|
|
||
| import FeedbackComponent from "../feedback-form/FeedbackComponent"; | ||
|
|
||
| function ErrorMessage(props: { imgSrc: string; imgAlt?: string; message: string }) { | ||
| return ( | ||
| <> | ||
| <div className="mt-16 flex space-x-2 py-6 px-6 w-96 justify-between bg-gitpod-kumquat-light rounded-xl"> | ||
| <div className="pr-3 self-center w-6"> | ||
| <img src={props.imgSrc} alt={props.imgAlt || "An error message"} /> | ||
| </div> | ||
| <div className="flex-1 flex flex-col"> | ||
| <p className="text-gitpod-red text-sm">{props.message}</p> | ||
| </div> | ||
| </div> | ||
| <FeedbackComponent | ||
| message={"Was this error message helpful?"} | ||
| initialSize={24} | ||
| isError={true} | ||
| isModal={false} | ||
| /> | ||
| </> | ||
| ); | ||
| } | ||
|
|
||
| export default ErrorMessage; |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,12 +11,23 @@ import meh from "../images/feedback/meh-emoji.svg"; | |||||||||
| import crying from "../images/feedback/crying-emoji.svg"; | ||||||||||
| import { trackEvent } from "../Analytics"; | ||||||||||
|
|
||||||||||
| function FeedbackComponent(props: { | ||||||||||
| onClose?: () => void; | ||||||||||
| isModal: boolean; | ||||||||||
| isError: boolean; | ||||||||||
| message?: string; | ||||||||||
| initialSize?: number; | ||||||||||
| }) { | ||||||||||
| const [text, setText] = useState<string>(""); | ||||||||||
| const [selectedEmoji, setSelectedEmoji] = useState<number | undefined>(); | ||||||||||
| const [isFeedbackSubmitted, setIsFeedbackSubmitted] = useState<boolean>(false); | ||||||||||
|
|
||||||||||
| const onClose = () => { | ||||||||||
| if (props.onClose) { | ||||||||||
| props.onClose(); | ||||||||||
| } | ||||||||||
|
Comment on lines
+26
to
+28
There was a problem hiding this comment. Can you use optional chaining here? There was a problem hiding this comment. Thanks for the review, @andrew-farries! I tried that but the linter still didn't like it 🙈 |
||||||||||
| setSelectedEmoji(undefined); | ||||||||||
| }; | ||||||||||
| const onSubmit = () => { | ||||||||||
| if (selectedEmoji) { | ||||||||||
| const feedbackObj = { | ||||||||||
|
|
@@ -28,7 +39,7 @@ function FeedbackComponent(props: { onClose: () => void; onSubmit: () => void; i | |||||||||
| trackEvent("feedback_submitted", feedbackObj); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| setIsFeedbackSubmitted(true); | ||||||||||
| }; | ||||||||||
|
|
||||||||||
| const handleClick = (emojiScore: number) => { | ||||||||||
|
|
@@ -54,12 +65,42 @@ function FeedbackComponent(props: { onClose: () => void; onSubmit: () => void; i | |||||||||
| </button> | ||||||||||
| )); | ||||||||||
| }; | ||||||||||
|
|
||||||||||
| const minimisedFirstView = !selectedEmoji && !isFeedbackSubmitted; | ||||||||||
| const expandedWithTextView = selectedEmoji && !isFeedbackSubmitted; | ||||||||||
|
|
||||||||||
| return ( | ||||||||||
| <> | ||||||||||
| {props.isModal && !isFeedbackSubmitted && <h3 className="mb-4">Send Feedback</h3>} | ||||||||||
| {minimisedFirstView && ( | ||||||||||
| <div | ||||||||||
| className={ | ||||||||||
| "flex flex-col justify-center px-6 py-4 border-gray-200 dark:border-gray-800 " + | ||||||||||
| (props.isError ? "mt-20 bg-gray-100 dark:bg-gray-800 rounded-xl" : "border-t") | ||||||||||
| } | ||||||||||
| > | ||||||||||
| <p | ||||||||||
| className={ | ||||||||||
| "text-center text-base mb-3 dark:text-gray-400 " + | ||||||||||
| (props.isError ? "text-gray-400" : "text-gray-500") | ||||||||||
| } | ||||||||||
| > | ||||||||||
| {props.message} | ||||||||||
| </p> | ||||||||||
|
|
||||||||||
| <div className="flex items-center justify-center w-full">{emojiGroup(props.initialSize || 50)}</div> | ||||||||||
| </div> | ||||||||||
| )} | ||||||||||
| {expandedWithTextView && ( | ||||||||||
| <> | ||||||||||
| <div | ||||||||||
| className={ | ||||||||||
| "flex flex-col px-6 py-4 border-gray-200 dark:border-gray-800 bg-white dark:bg-gray-900 " + | ||||||||||
|
There was a problem hiding this comment. suggestion: Minor color background and padding fix.
Suggested change
|
||||||||||
| (props.isError | ||||||||||
| ? "w-96 mt-6 bg-gray-100 dark:bg-gray-800 rounded-xl" | ||||||||||
|
There was a problem hiding this comment. suggestion: Minor bottom margin to always have some whitespace around the modal.
Suggested change
|
||||||||||
| : "-mx-6 border-t border-b") | ||||||||||
| } | ||||||||||
| > | ||||||||||
| <div className="relative"> | ||||||||||
| <div className="absolute flex bottom-5 right-5 -space-x-3">{emojiGroup(24)}</div> | ||||||||||
| <textarea | ||||||||||
|
There was a problem hiding this comment. suggestion: Some class changes to restore some color changes as I cannot add this code suggestion directly on the line. 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 |
||||||||||
|
|
@@ -82,26 +123,27 @@ function FeedbackComponent(props: { onClose: () => void; onSubmit: () => void; i | |||||||||
| . | ||||||||||
| </p> | ||||||||||
| </div> | ||||||||||
|
There was a problem hiding this comment. Restoring text alignment from #10071 (comment). Suggestion for https://github.com/gitpod-io/gitpod/pull/10071/files#diff-51811862e7996f416f9f7d73ff24341eecd96f3c083fb46d1b00b6cf94b50686R117: text-gray-500 text-left |
||||||||||
| <div className="flex justify-end mt-6"> | ||||||||||
| <button className="secondary" onClick={onClose}> | ||||||||||
| Cancel | ||||||||||
| </button> | ||||||||||
| <button className="ml-2" onClick={onSubmit}> | ||||||||||
| Send Feedback | ||||||||||
| </button> | ||||||||||
| </div> | ||||||||||
|
Comment on lines
+126
to
+133
There was a problem hiding this comment. 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.
|
||||||||||
| </div> | ||||||||||
| </> | ||||||||||
| )} | ||||||||||
| {isFeedbackSubmitted && ( | ||||||||||
| <div | ||||||||||
| className={ | ||||||||||
| "flex flex-col px-6 py-4 border-gray-200 dark:border-gray-800 " + | ||||||||||
| (props.isError ? "mt-20 bg-gray-100 dark:bg-gray-800 rounded-xl" : "") | ||||||||||
| } | ||||||||||
| > | ||||||||||
| <p className={"text-center text-base " + (props.isError ? "text-gray-400" : "text-gray-500")}> | ||||||||||
|
There was a problem hiding this comment. suggestion: In both cases, being able to dismiss the thanks message at the last step would be great to keep. |
||||||||||
| Thanks for your feedback, we appreciate it. | ||||||||||
laushinka marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. praise: Thanks for also including this step in the feedback modal used in the dashboard pages! 🙏 There was a problem hiding this comment. 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?
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. There was a problem hiding this comment. 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.
|
||||||||||
| </p> | ||||||||||
| </div> | ||||||||||
| )} | ||||||||||
| </> | ||||||||||
|
|
||||||||||
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.
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?
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.
Made a comment hereAh you already linked it. Yes that was the technical limitation from my end - would love some pointers 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.
@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.