Skip to content
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.

Sign up for GitHub

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

bug: Improved the feedback form UI issue #61 #75

Merged
merged 4 commits into from
Oct 7, 2024
Merged

bug: Improved the feedback form UI issue #61 #75

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4b9cb52
bug:improved the feedback form UI issue #61
vishnuprasad2004 Oct 4, 2024
4fc423e
aligned and centered
vishnuprasad2004 Oct 7, 2024
f4c60cb
Merge branch 'main' into feedback-ui
vishnuprasad2004 Oct 7, 2024
3873877
docs(contributor): contrib-readme-action has updated readme
github-actions[bot] Oct 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions frontend/src/App.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,18 @@ body,

body{
background: #F1EADC;
}

.rating-button {
@apply flex-grow bg-neutral-50 text-center p-2 rounded-md cursor-pointer select-none
}

input[type="radio"] {
display: none;
}

.star {
cursor: pointer;
font-size: 2rem;
margin-bottom: 0;
}
69 changes: 52 additions & 17 deletions frontend/src/components/ui/FeedbackForm.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { useState } from "react";
import { motion } from "framer-motion";
import { useInView } from "react-intersection-observer";
import chess from "../../assets/img/chess.gif";
import { FaStar } from "react-icons/fa6";

const FeedbackForm = () => {
const { ref, inView } = useInView({
Expand All @@ -18,15 +19,20 @@ const FeedbackForm = () => {
const [email, setEmail] = useState("");
const [feedback, setFeedback] = useState("");
const [submitted, setSubmitted] = useState(false);
const [rating, setRating] = useState(null);

const [hover, setHover] = useState(null);
const [totalStars, setTotalStars] = useState(5);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Simplify totalStars to a constant

Since totalStars represents a fixed value and setTotalStars is not used, you can replace the state variable with a constant to reduce unnecessary state management.

Apply this diff to simplify:

-const [totalStars, setTotalStars] = useState(5);
+const totalStars = 5;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const [totalStars, setTotalStars] = useState(5);
const totalStars = 5;

Comment on lines +23 to +26
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider optimizing totalStars state

The new state variables for rating and hover are correctly implemented. However, totalStars could be optimized:

const [rating, setRating] = useState(null);
const [hover, setHover] = useState(null);
const totalStars = 5; // Changed from useState to a constant

This optimization is beneficial because totalStars appears to be a fixed value that doesn't change during the component's lifecycle. Using a constant instead of a state variable can slightly improve performance and readability.


const handleSubmit = (e) => {
e.preventDefault();
console.log(`Name: ${name}, Email: ${email}, Feedback: ${feedback}`);
console.log(`Name: ${name}, Email: ${email}, Feedback: ${feedback}, rating: ${rating}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid logging sensitive user information to the console

Logging personal information such as names, emails, and feedback to the console can lead to security and privacy concerns, especially if the console output is exposed in production environments. Consider removing the console.log statement or ensure it's only used during development.

setSubmitted(true);
setTimeout(() => {
setName("");
setEmail("");
setFeedback("");
setRating(null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Reset the hover state after form submission

When resetting the form fields after submission, consider also resetting the hover state to prevent any lingering visual effects on the stars.

Apply this change to reset the hover state:

 setName("");
 setEmail("");
 setFeedback("");
 setRating(null);
+setHover(null);
 setSubmitted(false);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
setRating(null);
setName("");
setEmail("");
setFeedback("");
setRating(null);
setHover(null);
setSubmitted(false);

setSubmitted(false);
}, 3000);
};
Expand All @@ -42,10 +48,10 @@ const FeedbackForm = () => {
className="lg:grid lg:grid-cols-2 lg:gap-8"
>
<div className="mb-8 lg:mb-0 relative">
<h2 className="text-3xl font-extrabold text-[#004D43] sm:text-4xl">
We Value Your Feedback
<h2 className="text-5xl font-black text-[#004D43]">
We value Your Feedback!
</h2>
<p className="mt-4 text-lg text-gray-700 pb-3">
<p className="mt-1 text-lg text-gray-700 pb-3">
Your thoughts help us improve. Share your experience and
suggestions with us!
</p>
Expand All @@ -58,69 +64,98 @@ const FeedbackForm = () => {
</div>
</div>

<div className="mt-8 lg:mt-0">
<form onSubmit={handleSubmit} className="space-y-6">
<div className="bg-[#004D43] rounded-xl p-3 pt-4 h-fit">
<form onSubmit={handleSubmit} className="space-y-4">
<div>
<label
{/* <label
htmlFor="name"
className="block text-sm font-medium text-[#004D43]"
className="block text-sm font-medium text-white"
>
Name
</label>
</label> */}
Comment on lines +109 to +114
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Restore labels for form inputs to improve accessibility

Commenting out the labels for the Name, Email, and Feedback inputs can negatively impact accessibility. Screen readers rely on labels to convey the purpose of form controls to users with visual impairments. Placeholders are not a substitute for labels. Please restore the labels or use aria-label or aria-labelledby attributes to ensure the form is accessible to all users.

Also applies to: 87-92, 104-109

<input
type="text"
id="name"
value={name}
placeholder="Name"
onChange={(e) => setName(e.target.value)}
required
className="mt-1 block w-full border border-gray-300 rounded-md shadow-sm py-2 px-3 focus:outline-none focus:ring-[#004D43] focus:border-[#004D43]"
/>
</div>
<div>
<label
{/* <label
htmlFor="email"
className="block text-sm font-medium text-[#004D43]"
>
Email
</label>
</label> */}
<input
type="email"
id="email"
placeholder="Email"
value={email}
onChange={(e) => setEmail(e.target.value)}
required
className="mt-1 block w-full border border-gray-300 rounded-md shadow-sm py-2 px-3 focus:outline-none focus:ring-[#004D43] focus:border-[#004D43]"
/>
</div>
<div>
<label
{/* <label
htmlFor="feedback"
className="block text-sm font-medium text-[#004D43]"
>
Feedback
</label>
</label> */}
<textarea
id="feedback"
placeholder="Your valuable feedback here"
rows="4"
value={feedback}
onChange={(e) => setFeedback(e.target.value)}
required
className="mt-1 block w-full border border-gray-300 rounded-md shadow-sm py-2 px-3 focus:outline-none focus:ring-[#004D43] focus:border-[#004D43]"
className="mt-1 block w-full border border-gray-300 rounded-md shadow-sm py-2 px-3 focus:outline-none focus:ring-[#004D43] focus:border-[#004D43] resize-none"
Comment on lines +106 to +156
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improve form accessibility

While the styling changes and added placeholders enhance the form's appearance, commenting out the labels (lines 109-114, 126-131, 143-148) negatively impacts accessibility. Consider these options:

  1. Restore the labels for better accessibility:
<label htmlFor="name" className="sr-only">Name</label>
<input
  type="text"
  id="name"
  value={name}
  placeholder="Name"
  // ... other props
/>
  1. Use aria-label attributes if you prefer not to use visible labels:
<input
  type="text"
  id="name"
  value={name}
  placeholder="Name"
  aria-label="Name"
  // ... other props
/>

These changes will ensure that screen readers can properly convey the purpose of each form field to users with visual impairments.

></textarea>
</div>
<div className="flex flex-row justify-center gap-2">
{[...Array(totalStars)].map((star, index) => {
const currentRating = index + 1;
return (
<label key={index}>
<input
type="radio"
name="rating"
value={currentRating}
onChange={() => setRating(currentRating)}
/>
<span
className="star"
style={{
color:
currentRating <= (hover || rating) ? "#ffc107" : "#e4e5e9"
}}
onMouseEnter={() => setHover(currentRating)}
onMouseLeave={() => setHover(null)}
>
<FaStar />
</span>
</label>
);
})}
</div>
Comment on lines +159 to +184
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance accessibility and usability of star rating component

The star rating component is a good addition, but it needs improvements for accessibility and usability:

  1. Add aria-label to provide context for screen readers:
<input
  type="radio"
  name="rating"
  value={currentRating}
  onChange={() => setRating(currentRating)}
  aria-label={`Rate ${currentRating} out of ${totalStars} stars`}
/>
  1. Make the stars focusable and selectable via keyboard:
<span
  className="star"
  style={{
    color: currentRating <= (hover || rating) ? "#ffc107" : "#e4e5e9"
  }}
  onMouseEnter={() => setHover(currentRating)}
  onMouseLeave={() => setHover(null)}
  onKeyDown={(e) => {
    if (e.key === 'Enter' || e.key === ' ') {
      setRating(currentRating);
    }
  }}
  tabIndex={0}
  role="button"
>
  <FaStar />
</span>
  1. Consider adding a visual or text indication of the current rating for better user feedback.

These changes will significantly improve the accessibility and usability of the star rating component.

<div>
<button
type="submit"
className="w-full flex justify-center py-2 px-4 border border-transparent rounded-md shadow-sm text-sm font-medium text-white bg-[#004D43] hover:bg-[#003d35] focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-[#004D43]"
className="w-full flex justify-center py-2 px-4 border border-transparent rounded-md shadow-sm text-sm font-medium text-white bg-[#09342e] hover:bg-[#072d28] focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-[#004D43]"
>
Submit Feedback
</button>
</div>
</form>
{submitted && (
<motion.div
initial={{ opacity: 0, y: -10 }}
animate={{ opacity: 1, y: 0 }}
initial={{ opacity: 0, y: -10, display:"none", height:0 }}
animate={{ opacity: 1, y: 0, display: "block", height: "auto" }}
Comment on lines +196 to +197
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve animation of feedback confirmation message

The current animation approach using display property can lead to layout shifts. Consider using opacity and transform for smoother animations:

<motion.div
  initial={{ opacity: 0, y: -10 }}
  animate={{ opacity: 1, y: 0 }}
  exit={{ opacity: 0, y: -10 }}
  transition={{ duration: 0.3 }}
  className="mt-4 p-4 bg-green-100 border border-green-400 text-green-700 rounded"
>
  Thank you for your feedback!
</motion.div>

Also, consider wrapping this component with Framer Motion's AnimatePresence to handle enter/exit animations smoothly:

import { AnimatePresence } from 'framer-motion';

// In your JSX
<AnimatePresence>
  {submitted && (
    <motion.div
      // ... animation props as above
    >
      Thank you for your feedback!
    </motion.div>
  )}
</AnimatePresence>

This approach provides a smoother animation without causing layout shifts.

className="mt-4 p-4 bg-green-100 border border-green-400 text-green-700 rounded"
>
Thank you for your feedback!
Expand Down
Loading