-
-
Notifications
You must be signed in to change notification settings - Fork 96
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
feat: Added frontend for newsletter section ( exploring backend for the same ) #331
Conversation
@Adeesh-bode is attempting to deploy a commit to the Sanchit Bajaj's projects Team on Vercel. A member of the Team first needs to authorize it. |
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.
Great job, @Adeesh-bode! 🎉 Thank you for submitting your pull request. Your contribution is valuable and we appreciate your efforts to improve our project. Keep being the part of the community!
'We will promptly review your changes and offer feedback. Keep up the excellent work!
Kindly remember to check our Contributing Guidelines'
WalkthroughThe recent changes introduce a new Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant HomePage
participant Newsletter
participant ButtonLong
User->>HomePage: Load Home Page
HomePage->>Newsletter: Render Newsletter Component
User->>Newsletter: Enter Email and Submit
Newsletter->>Newsletter: handleChange and handleSubmit
Newsletter->>ButtonLong: Render Submit Button
ButtonLong->>User: Display Button
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/components/core/newsletter/index.tsx (1 hunks)
- src/components/pages/home/index.tsx (2 hunks)
- src/styles/globals.css (1 hunks)
Files skipped from review due to trivial changes (3)
- src/components/core/newsletter/index.tsx
- src/components/pages/home/index.tsx
- src/styles/globals.css
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.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/components/core/newsletter/index.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/components/core/newsletter/index.tsx
const Newsletter: React.FC = () => { | ||
return <div className="w-full "> | ||
<div className="flex flex-col justify-center items-center gap-6 px-20 py-10 "> | ||
<h4 className="text-xs uppercase">Newsletter</h4> |
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.
The font size is way too small
<div className="flex flex-col justify-center items-center gap-6 px-20 py-10 "> | ||
<h4 className="text-xs uppercase">Newsletter</h4> | ||
<h2 className="text-xs md:text-base uppercase text-primary-light text-center">Sign up for latest updates and offers</h2> | ||
<div className="flex flex-col gap-4 md:flex-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.
In light mode, it is unclear if an input is even present, put something like a border around it for light mode.
I feel that the backend also has to be included in this PR to merge this into main. We can't ship incomplete features, @Sanchitbajaj02 thoughts? |
Yes, backend is must. Also, @Adeesh-bode there is no need to create new button, I have already configured a customized button with an action, please use that. |
@Adeesh-bode any updates on this PR? |
I thought you rejected my work so I wasn't working on it... is it possible if only frontend part is done by me ( I would explore app write and do the if not possible ). I am asking cause a person familiar with app write can do this job better. |
I never mentioned the rejection. If you scroll up we suggested the changes that needs to be done to get it merged. Also it is alright if you don't know or not familiar with Appwrite. The PR will be merged, just do the changes that are suggested above |
ok I will do the suggested changes and raise pr by day end.. |
hey sanchit , I started working on using already made custom button , which is used for redirection by href but the subscribe button is not redirecting it is triggering and onSubmit action for backend. I will still want to use your made custom button than I would have to make states which will pass from child component to parent i.e from custom button component to newsletter component ( which is not recommend ) or will have to use redux global state . If you say so I will work on it, as I looked in other part of project like login & register page where such trigger buttons are used , they also didn't used custom button. |
other than button suggestion other frontend suggestion are considered. |
For the button, you can introduce an option on click event field. Like either the href will work or the onclick event will work on that button. This will help streamline the button component throughout the application |
done, utilized the prebuilt ui component for button |
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.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/components/core/buttons/index.tsx (1 hunks)
- src/components/core/newsletter/index.tsx (1 hunks)
Additional context used
Biome
src/components/core/newsletter/index.tsx
[error] 28-28: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Additional comments not posted (3)
src/components/core/buttons/index.tsx (2)
5-9
: The addition of thetype
property enhances the flexibility of theButton
interface, allowing it to accommodate various user interactions. Good enhancement!
12-28
: The conditional rendering inButtonLong
to support both<button>
and<Link>
elements based on thetype
property is well-implemented. This change makes the component versatile for different contexts.src/components/core/newsletter/index.tsx (1)
6-43
: The implementation of theNewsletter
component is comprehensive, handling user input and form submission effectively. The use of social media icons adds a nice touch for user engagement.
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.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/components/core/newsletter/index.tsx (1 hunks)
Additional context used
Biome
src/components/core/newsletter/index.tsx
[error] 1-2: The default import is only used as a type.
Additional comments not posted (2)
src/components/core/newsletter/index.tsx (2)
7-11
: State management for subscriber email is implemented correctly.
42-42
: Export of theNewsletter
component is correctly implemented.
I didn't know app write backend so I asked sanchit , that can I do the frontend. So I did only frontend part. The image for frontend is attached in above message |
Related Issue
Issue Number : #295
Part of GirlScript summer of code
Added frontent for newletter section
Screenshots
Attached
Summary by CodeRabbit
New Features
Newsletter
component for email sign-ups and social media sharing.type
property to buttons, allowing for different button types (button, submit, reset).Style