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

changes made in eye icon #924

Merged
merged 4 commits into from
Jul 24, 2023
Merged

changes made in eye icon #924

merged 4 commits into from
Jul 24, 2023

Conversation

shruuti321
Copy link
Contributor

@shruuti321 shruuti321 commented Jul 23, 2023

Closes #907
I have restyled the eye icon by changing background color,icon color and increasing icon size in both login and signup pages.
Kindly review and merge
image
image

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 shruuti321! for creating this pull request and contributing to Dummygram! 💗

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! 😀

@shruuti321
Copy link
Contributor Author

@narayan954 please review the pr

@narayan954
Copy link
Owner

Closes #907 I have restyled the eye icon by changing background color,icon color and increasing icon size in both login and signup pages. Kindly review and merge image image

Hi @shruuti321 thanks for enhancing it, but as you see in the third picture, it's not centered properly, can you try to fix that, please?

@shruuti321
Copy link
Contributor Author

@narayan954
Yes I tried several properties like margin-left:auto,margin-right:auto, text-align:center,align-items:center
but nothing is working.
using display:flex got me this
image

but this has disturbed the alignment in signup page
image
So kindly suggest whether to make this change or keep it as previous

@narayan954
Copy link
Owner

@narayan954 Yes I tried several properties like margin-left:auto,margin-right:auto, text-align:center,align-items:center but nothing is working. using display:flex got me this image

but this has disturbed the alignment in signup page image So kindly suggest whether to make this change or keep it as previous

Does separating this css or adding inline css just for this element work? If not, then it's alright, I'll allow this

@shruuti321
Copy link
Contributor Author

@narayan954 Yes I tried several properties like margin-left:auto,margin-right:auto, text-align:center,align-items:center but nothing is working. using display:flex got me this image
but this has disturbed the alignment in signup page image So kindly suggest whether to make this change or keep it as previous

Does separating this css or adding inline css just for this element work? If not, then it's alright, I'll allow this

Not working, tried that as well

@narayan954
Copy link
Owner

@narayan954 Yes I tried several properties like margin-left:auto,margin-right:auto, text-align:center,align-items:center but nothing is working. using display:flex got me this image
but this has disturbed the alignment in signup page image So kindly suggest whether to make this change or keep it as previous

Does separating this css or adding inline css just for this element work? If not, then it's alright, I'll allow this

Not working, tried that as well

Alright

Copy link
Owner

@narayan954 narayan954 left a comment

Choose a reason for hiding this comment

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

looks good to me!

@narayan954 narayan954 merged commit 84c020f into narayan954:master Jul 24, 2023
3 of 4 checks passed
@shruuti321
Copy link
Contributor Author

@narayan954 may i know if you have approved the changes as proposed previously or the recent ones?
I havent created a pull request for the last change

@narayan954 Yes I tried several properties like margin-left:auto,margin-right:auto, text-align:center,align-items:center but nothing is working. using display:flex got me this image
but this has disturbed the alignment in signup page image So kindly suggest whether to make this change or keep it as previous

Does separating this css or adding inline css just for this element work? If not, then it's alright, I'll allow this

Not working, tried that as well

May i know if you have approved the changes I have made earlier or the recent ones, because I haven't created a pull request for the recent changes yet

@narayan954
Copy link
Owner

@narayan954 may i know if you have approved the changes as proposed previously or the recent ones?
I havent created a pull request for the last change

@narayan954 Yes I tried several properties like margin-left:auto,margin-right:auto, text-align:center,align-items:center but nothing is working. using display:flex got me this image
but this has disturbed the alignment in signup page image So kindly suggest whether to make this change or keep it as previous

Does separating this css or adding inline css just for this element work? If not, then it's alright, I'll allow this

Not working, tried that as well

May i know if you have approved the changes I have made earlier or the recent ones, because I haven't created a pull request for the recent changes yet

I went with the original changes.

Also fyi you don't need to create a new pull request if you want to add changes to this pr... you can directly commit and push and this pr will be updated with the commits automatically :)

@shruuti321
Copy link
Contributor Author

@narayan954 may i know if you have approved the changes as proposed previously or the recent ones?
I havent created a pull request for the last change

@narayan954 Yes I tried several properties like margin-left:auto,margin-right:auto, text-align:center,align-items:center but nothing is working. using display:flex got me this image
but this has disturbed the alignment in signup page image So kindly suggest whether to make this change or keep it as previous

Does separating this css or adding inline css just for this element work? If not, then it's alright, I'll allow this

Not working, tried that as well

May i know if you have approved the changes I have made earlier or the recent ones, because I haven't created a pull request for the recent changes yet

I went with the original changes.

Also fyi you don't need to create a new pull request if you want to add changes to this pr... you can directly commit and push and this pr will be updated with the commits automatically :)

Okay! Thanks a lot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Regarding the styling of eye icon in the password field
2 participants