UI/contact us #78
UI/contact us #78
Conversation
|
@sub1120 please have a look and tell me the review so that I can move with other stuffs |
Hello @Vinay-Pratap-Singh, Thanks for your active contribution. Sorry, but this PR is too long to review. It is always easier to review and merge small PRs, and If we move this fast, we may end up missing small things behind. Could you please create separate PRs for different sections of UI? Also, we are planning to setup testing before we move further in UI development. I have already created an issue #73 for it, if you or any of your friend have any idea about "How to setup UI Testing", please have a look upon it. |
I like this idea, but let it slide this one time since it is already done. Let's review this and merge and from next time onwards @Vinay-Pratap-Singh please keep the PRs short. |
Okay, I will try to review it as soon as possible. |
Thanks @shivamvijaywargi and @sub1120 for considering it, from next time i will try to make pr for each component. |
|
@shivamvijaywargi please have a look into it and let me know if any further improvement is required. |
|
@sub1120 please take it over. |
checking... |
|
@Vinay-Pratap-Singh, please check few requested UI changes from my side. Apart from that all good. Nice work. |
There was a problem hiding this comment.
Vinay, below UI looks little different from design (spacing and separator). Could you please compare and check once. For sperator please use shadcn seperator
| Current | Design |
|---|---|
There was a problem hiding this comment.
done have a look @sub1120
There was a problem hiding this comment.
Can we use separator component provided by shandcn instead of border.
There was a problem hiding this comment.
I don't think it is required, if I can do that using the border then why use a separator over there unnecessarily?
Even if you want me to use a separator then can you please provide me any specific reason either in the name of performance, good practice, or from any relevant resource that using a separator is better instead of a border which is an inbuilt property of the CSS.
There was a problem hiding this comment.
Generally shandcn components provide better accessibility.
Check out this separator code to know how it is different from our current implementation.
https://github.com/radix-ui/primitives/blob/main/packages/react/separator/src/Separator.tsx
So let's try whenever possible use shadcn components.
There was a problem hiding this comment.
Thanks for the reference, I also understand the accessibility part but not convinced for this particular thing that I should remove the border and replace it with the separator code.
Because the argument from your end is not sufficient to which you are giving to change the code.
I accepted the required changes and modified the code as per your concerns which were really good and important but I do not think the same for this.
By the way, I am not going to modify this code, you guys can reject this PR as you guys are the owner and have complete authority over how you want this project to be.
Thanks for all the PRs to which you guys have merged by which I had got a good experience and was an amazing journey. From now onward I will not be able to contribute more to this project.
Thanks to @shivamvijaywargi and @sub1120 for giving me the chance to contribute to this project.
There was a problem hiding this comment.
@Vinay-Pratap-Singh we are just trying to maintain best practices in this project and accessibility is one of it. If we maintain code standards from the beginning, it will allow other collaborators to follow the same.
I know this PR took more than the usual time for review. But believe us, we are just trying our best to maintain the codebase. You are among the top contributors in this project. We would love to continue with you, but it's totally your choice.🙂
Describe your changes
Created the contact us page
Issue ticket number and link
#76 #77 #75 #74
Please confirm the below message,