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 icons to share options list and added an alert box for sign in #23

Merged
merged 3 commits into from
Oct 11, 2021

Conversation

kritikash18
Copy link
Contributor

  1. Currently, when clicking on sign in with blank username and room, nothing happens. For that, added an alert box for user to specify room and username as shown below

Screenshot 2021-10-09 at 5 47 39 PM

  1. Currently, the share list looks like this:

Screenshot 2021-10-09 at 5 44 01 PM

Updated it with icons for each option, so it looks like this:
Screenshot 2021-10-09 at 5 47 59 PM

@vercel
Copy link

vercel bot commented Oct 9, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

chatcus – ./client

🔍 Inspect: https://vercel.com/manuelalferez/chatcus/92hcFPLXyiyhGfaSW3ckv54EjX1p
✅ Preview: https://chatcus-git-fork-kritikash18-iconsshare-manuelalferez.vercel.app

[Deployment for b5ad308 failed]

chat – ./client

🔍 Inspect: https://vercel.com/manuelalferez/chat/2Zf2NMc1FfCKLfTtHyix7TcUNesB
✅ Preview: https://chat-git-fork-kritikash18-iconsshare-manuelalferez.vercel.app

[Deployment for b5ad308 failed]

@kritikash18
Copy link
Contributor Author

Fixes #24

@iamvishal345
Copy link
Collaborator

iamvishal345 commented Oct 10, 2021

@kritikash18 , This is good UI change. I have some suggestions for this change

  1. Currently the icon and menu text is not center aligned.
  2. You have you png images . Please replace these with svg. You can find svg icons here.
  3. Also may be add a hover effect and change icon color according to logo.

Regarding alert box, this is also great.
I have added a modal component in latest PR so you can use that one instead of default alert box.

@iamvishal345 iamvishal345 linked an issue Oct 10, 2021 that may be closed by this pull request
@kritikash18
Copy link
Contributor Author

@manuelalferez @iamvishal345 I updated the icons to SVG and center aligned the content. After alignment, which one of the following do you think looks better?

With names:
Screenshot 2021-10-11 at 3 43 43 PM

Without names:
Screenshot 2021-10-11 at 3 44 09 PM

@iamvishal345
Copy link
Collaborator

iamvishal345 commented Oct 11, 2021

@manuelalferez @iamvishal345 I updated the icons to SVG and center aligned the content. After alignment, which one of the following do you think looks better?

With names: Screenshot 2021-10-11 at 3 43 43 PM

Without names: Screenshot 2021-10-11 at 3 44 09 PM

@kritikash18 I think vertical align is fine. Keep horizontal align as space between.
image
Something like this.
For more reference checkout the Copy Link option in share dropdown of articles in dev.to

@manuelalferez
Copy link
Owner

I see everything perfect. Thanks @kritikash18 for the new feature and @iamvishal345 for the review.

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.

Added icons to share list and added an alert box
3 participants