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.

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

Radiobutton component #90

Merged
merged 26 commits into from
Aug 28, 2024
Merged

Radiobutton component #90

merged 26 commits into from
Aug 28, 2024

Conversation

KenanTopal
Copy link
Contributor

No description provided.

frontend/src/components/CheckIcon/CheckIcon.jsx Outdated Show resolved Hide resolved
frontend/src/components/CheckIcon/CheckIcon.jsx Outdated Show resolved Hide resolved
frontend/src/scenes/dashboard/Dashboard.jsx Outdated Show resolved Hide resolved
frontend/src/components/RadioButton/RadioButton.jsx Outdated Show resolved Hide resolved
Copy link
Contributor

@uparkalau uparkalau left a comment

Choose a reason for hiding this comment

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

Please implement requested changes.

uparkalau
uparkalau previously approved these changes Jul 12, 2024
erenfn
erenfn previously requested changes Jul 20, 2024
frontend/src/components/CheckIcon/CheckIcon.jsx Outdated Show resolved Hide resolved
frontend/src/components/CheckIcon/CheckIcon.jsx Outdated Show resolved Hide resolved
uparkalau
uparkalau previously approved these changes Jul 20, 2024
@uparkalau uparkalau requested review from erenfn and uparkalau July 26, 2024 14:31
@SimerdeepSinghGrewal
Copy link
Contributor

I've done these things
added inline style (basic, required everywhere) for radio button but can be removed.
directly applied color prop to style and used default color, earlier it was using conditional logic for default color
used checked prop
removed 'isRequired' from propTypes

Copy link
Collaborator

@erenfn erenfn left a comment

Choose a reason for hiding this comment

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

image

  • You can't click and then unclick the button; it remains unclicked.
  • Text continues in new line. It shouldn't.
  • In cases where there are multiple radio buttons that depend on each other (for example, on the Create Banner Page, there are two radio buttons: top and bottom), add functionality so that when one is clicked, the other is unselected. I leave the implementation to you.

@SimerdeepSinghGrewal
Copy link
Contributor

image

  • You can't click and then unclick the button; it remains unclicked.
  • Text continues in new line. It shouldn't.
  • In cases where there are multiple radio buttons that depend on each other (for example, on the Create Banner Page, there are two radio buttons: top and bottom), add functionality so that when one is clicked, the other is unselected. I leave the implementation to you.

done these and for functionality you need to provide same "name" attribute to both radio buttons.

@erenfn
Copy link
Collaborator

erenfn commented Aug 26, 2024

Use our RadioButton in components/BannerPageContents/BannerLeftContent.jsx to fit the style and functionality. Do git pull. I have pushed some changes to BannerLeftContent.jsx to let you understand what we expect. After you integrate our RadioButton, delete unnecessary code that I added there.

That page is in http://localhost:5173/banner/create after you run npm run dev you can navigate there to check the functionality

@uparkalau uparkalau requested a review from erenfn August 27, 2024 14:46
@SimerdeepSinghGrewal
Copy link
Contributor

Use our RadioButton in components/BannerPageContents/BannerLeftContent.jsx to fit the style and functionality. Do git pull. I have pushed some changes to BannerLeftContent.jsx to let you understand what we expect. After you integrate our RadioButton, delete unnecessary code that I added there.

That page is in http://localhost:5173/banner/create after you run npm run dev you can navigate there to check the functionality

I removed MUI's Radio, RadioGroup and FormControlLabel from BannerLeftContent.jsx and updated the functionality(as required) with RadioButton component.
Updated the RadioButton component to mimic Figma design - used the styling from MUI website. Its looks like this now
Screenshot 2024-08-27 134043

frontend/src/App.jsx Outdated Show resolved Hide resolved
uparkalau
uparkalau previously approved these changes Aug 27, 2024
@erenfn erenfn merged commit 2d3d48c into develop Aug 28, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants