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

Added CTA And Made Sidebar take whole space #1527

Closed
wants to merge 1 commit into from

Conversation

Souravvmishra
Copy link

@Souravvmishra Souravvmishra commented Aug 9, 2023

Fixes

Closes #1448

Changes proposed

  1. Added CTA
  2. Made Sidebar Take Full Space

Screenshots

Screen Shot 2023-08-09 at 11 13 37

Screen Shot 2023-08-09 at 11 13 44

Note to reviewers

@vercel
Copy link

vercel bot commented Aug 9, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
linkshub ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 9, 2023 5:46am

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you, Souravvmishra, for creating this pull request and contributing to LinksHub! 💗

The maintainers will review this Pull Request and provide feedback as soon as possible! 😇
We appreciate your patience and contribution, Keep up the great work! 😀

@Souravvmishra
Copy link
Author

@rupali-codes Reviews are appreciated

Copy link
Collaborator

@CBID2 CBID2 left a comment

Choose a reason for hiding this comment

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

I could not see the changes when I went to the preview site @Souravvmishra(see screenshot below). Can you fix this?
no sidebar

@CBID2 CBID2 requested a review from k-deepak04 August 10, 2023 01:50
@CBID2 CBID2 added quick-fix Shouldn't take much time to finish gssoc GirlScript Summer of Code participants level2 Modifying an existing feature labels Aug 10, 2023
@Souravvmishra
Copy link
Author

This is because it is available only on small screens devices.

Copy link
Collaborator

@CBID2 CBID2 left a comment

Choose a reason for hiding this comment

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

LGTM

@CBID2 CBID2 added goal: new-feature New feature or request status: ready-to-merge Approved & its ready-to-merge labels Aug 10, 2023
@k-deepak04
Copy link
Contributor

@rupali-codes @CBID2 as far my experience it's not that much suitable to add full side panel that covers the whole screen

image
image
image

as you can see this big brands also use the 70% ration of the hamburger menu......
rest your call........this is my opinion

@CBID2
Copy link
Collaborator

CBID2 commented Aug 10, 2023

Did you do it on a small screen @k-deepak04? That's what it's for.

@k-deepak04
Copy link
Contributor

Did you do it on a small screen @k-deepak04? That's what it's for.

yep christy all the ss are from small screens only
mobile tablets comes under small screens category
and i have added ss of those.......

@CBID2
Copy link
Collaborator

CBID2 commented Aug 10, 2023

Did you do it on a small screen @k-deepak04? That's what it's for.

yep christy all the ss are from small screens only

mobile tablets comes under small screens category

and i have added ss of those.......

Ah ok. It seemed to work on my phone.

@rupali-codes
Copy link
Owner

@Souravvmishra hi thanks. Can you make it for mobile screens only? not for tablet and all?

@Souravvmishra
Copy link
Author

Its build as per the design when the sidebar becomes hidden, we show a explore button.

@Souravvmishra
Copy link
Author

@rupali-codes @CBID2 as far my experience it's not that much suitable to add full side panel that covers the whole screen

image
image
image

as you can see this big brands also use the 70% ration of the hamburger menu......
rest your call........this is my opinion

These Big Brands are using the sidebar as a sidebar for extra options but in our case, our sidebar has all the Focus of the user

@k-deepak04
Copy link
Contributor

@rupali-codes @CBID2 as far my experience it's not that much suitable to add full side panel that covers the whole screen
image
image
image
as you can see this big brands also use the 70% ration of the hamburger menu......
rest your call........this is my opinion

These Big Brands are using the sidebar as a sidebar for extra options but in our case, our sidebar has all the Focus of the user

okay man :)
no issues just make that for mobile not for tablets as said by rupali then we are good to go

@Souravvmishra
Copy link
Author

Its build as per the design when the sidebar becomes hidden, we show a explore button.

Are You Sure You Want To Remove It For Tablets?
In tablets, the sidebar is hidden, So We Need The CTA

@Souravvmishra
Copy link
Author

can you please merge this

@CBID2
Copy link
Collaborator

CBID2 commented Aug 11, 2023

@rupali-codes?

@rupali-codes
Copy link
Owner

@Souravvmishra can we do it only for mobile screens? not for tablets and all?

@CBID2 CBID2 removed the gssoc GirlScript Summer of Code participants label Aug 19, 2023
@CBID2 CBID2 requested review from Anmol-Baranwal and removed request for ujjawaltyagii August 19, 2023 23:54
Copy link
Collaborator

@Anmol-Baranwal Anmol-Baranwal left a comment

Choose a reason for hiding this comment

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

It should be for small screen devices only.

Also, can you explain why full screen sidebar is better? I don't want assumptions. Say from the perspective of a user.

Copy link
Contributor

@k-deepak04 k-deepak04 left a comment

Choose a reason for hiding this comment

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

@Souravvmishra please tell why full screen is needed , as rightly said by anmol also have you did only for mobile not for tabs as requested by rupali?
If not please do it

@Anmol-Baranwal
Copy link
Collaborator

@Souravvmishra
If you are not interested in solving the issue. Kindly let me know, so that I can close the pull request.

@Souravvmishra
Copy link
Author

Apologies for the late reply, I am on a trip.

Why do we need a full screen?
Because a semi sidebar gives the perception of a less important component. True that many big companies use a semi sidebar but they simply use that as a sidebar for some extra things but in our case, Sidebar is the main component. We need maximum importance to that.

Actually I would Suggest Creating a new page instead of just using the sidebar

These all work for small screens only as in the large screen the sidebar is already visible. So at least the user is aware of it. But in small screen, the user has no idea that they are in sidebar. Yes it is written but who reads?

@Anmol-Baranwal
Copy link
Collaborator

Apologies for the late reply, I am on a trip.

Why do we need a full screen? Because a semi sidebar gives the perception of a less important component. True that many big companies use a semi sidebar but they simply use that as a sidebar for some extra things but in our case, Sidebar is the main component. We need maximum importance to that.

Actually I would Suggest Creating a new page instead of just using the sidebar

These all work for small screens only as in the large screen the sidebar is already visible. So at least the user is aware of it. But in small screen, the user has no idea that they are in sidebar. Yes it is written but who reads?

Feel free to reply whenever you have time. No rush!

I don't understand this:

But in small screen, the user has no idea that they are in sidebar. Yes it is written but who reads?

@rupali-codes @CBID2 @k-deepak04
What do guys think?

@CBID2
Copy link
Collaborator

CBID2 commented Aug 25, 2023

Apologies for the late reply, I am on a trip.

Why do we need a full screen? Because a semi sidebar gives the perception of a less important component. True that many big companies use a semi sidebar but they simply use that as a sidebar for some extra things but in our case, Sidebar is the main component. We need maximum importance to that.

Actually I would Suggest Creating a new page instead of just using the sidebar

These all work for small screens only as in the large screen the sidebar is already visible. So at least the user is aware of it. But in small screen, the user has no idea that they are in sidebar. Yes it is written but who reads?

Feel free to reply whenever you have time. No rush!

I don't understand this:

But in small screen, the user has no idea that they are in sidebar. Yes it is written but who reads?

@rupali-codes @CBID2 @k-deepak04

What do guys think?

I'm lost too @Anmol-Baranwal

@k-deepak04
Copy link
Contributor

Apologies for the late reply, I am on a trip.

Why do we need a full screen? Because a semi sidebar gives the perception of a less important component. True that many big companies use a semi sidebar but they simply use that as a sidebar for some extra things but in our case, Sidebar is the main component. We need maximum importance to that.

Actually I would Suggest Creating a new page instead of just using the sidebar

These all work for small screens only as in the large screen the sidebar is already visible. So at least the user is aware of it. But in small screen, the user has no idea that they are in sidebar. Yes it is written but who reads?

Feel free to reply whenever you have time. No rush!
I don't understand this:

But in small screen, the user has no idea that they are in sidebar. Yes it is written but who reads?

@rupali-codes @CBID2 @k-deepak04
What do guys think?

I'm lost too @Anmol-Baranwal

I think we should have one landing page then one for the categories from where user can go to the resources.
We had a discussion on it too right @rupali-codes ?

@rupali-codes
Copy link
Owner

@k-deepak04 yes we did, but still, how we actually going render resources in there? its not like 5-10 resources that user can directly see on the screen, they will have to scroll. And having a landing page is completely a sperate idea, it was for where we can show other things, like About LinksHub, Contacting us, How people can contribute, showing contributors list etc etc.

Having the sidebar is not a problem, we just need to make it more accessible.

@Anmol-Baranwal & @CBID2 what are your takes on this?

Copy link
Owner

@rupali-codes rupali-codes left a comment

Choose a reason for hiding this comment

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

@Souravvmishra its showing 2 scrollbars in the sidebar, could you please check?

image

@Anmol-Baranwal
Copy link
Collaborator

@rupali-codes
When I made the PR to fix this searchbar, there was only one sidebar (which was very tough), but I did implement that.

Later, someone must have changed the code which removed that functionality.

See this: https://linkshub-1arqvr3it-rupali-codes.vercel.app/

Like, I did it properly.

@CBID2
Copy link
Collaborator

CBID2 commented Aug 26, 2023

@k-deepak04 yes we did, but still, how we actually going render resources in there? its not like 5-10 resources that user can directly see on the screen, they will have to scroll. And having a landing page is completely a sperate idea, it was for where we can show other things, like About LinksHub, Contacting us, How people can contribute, showing contributors list etc etc.

Having the sidebar is not a problem, we just need to make it more accessible.

@Anmol-Baranwal & @CBID2 what are your takes on this?

I agree @rupali-codes

@Anmol-Baranwal Anmol-Baranwal removed the status: ready-to-merge Approved & its ready-to-merge label Aug 29, 2023
@CBID2
Copy link
Collaborator

CBID2 commented Sep 4, 2023

How are we going to move forward with this @rupali-codes and @Anmol-Baranwal?

@Anmol-Baranwal
Copy link
Collaborator

Don't know, @rupali-codes must be active.

@rupali-codes
Copy link
Owner

Don't know, @rupali-codes must be active.

can we just keep this PR to add CTA and keep the sidebar as it is currently?

@CBID2
Copy link
Collaborator

CBID2 commented Sep 5, 2023

Don't know, @rupali-codes must be active.

can we just keep this PR to add CTA and keep the sidebar as it is currently?

That's not a bad idea @rupali-codes. What do you think @Anmol-Baranwal?

@CBID2 CBID2 closed this Sep 17, 2023
@CBID2
Copy link
Collaborator

CBID2 commented Sep 17, 2023

Closed due to inactivity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
goal: new-feature New feature or request level2 Modifying an existing feature quick-fix Shouldn't take much time to finish
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding A CTA Button & Making The Sidebar Take Full Space
5 participants