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

feat: 🍰 Show Password Component #4370

Merged
merged 5 commits into from
May 18, 2021
Merged

Conversation

Brandon-G-Tripp
Copy link
Contributor

@Brandon-G-Tripp Brandon-G-Tripp commented Apr 14, 2021

🍰 Pullrequest

Pull request pulls out logic for the ShowPassword into a component for use in various password fields.

Issues

#4312

Todo

  • Implement in Registration Slider

@ulfgebhardt ulfgebhardt added this to the 🏃 21/04 April milestone Apr 15, 2021
@Tirokk Tirokk changed the title Show Password Component feat: 🍰 Show Password Component Apr 15, 2021
@Tirokk
Copy link
Member

Tirokk commented Apr 15, 2021

That looks really good @Brandon-G-Tripp ! 🍀👍🏼
Nice that you care for this again.

And I very much appreciate that you wrote a test for it. 😍

I just see one little leftover from last time at the moment:

Bildschirmfoto 2021-04-15 um 14 20 26

Not very important, but would be nice if you could care for this little double boarder to vanish …

PS: Just in case you are interested to write a Cypress end-to-end test.
After @ulfgebhardt merges his Cypress PR #4338 pull down the newest master and have a look into the cypress folder. There are as well some instructions how to run the tests.

@Tirokk Tirokk marked this pull request as draft April 15, 2021 12:53
@Tirokk Tirokk changed the title feat: 🍰 Show Password Component feat: [WIP] 🍰 Show Password Component Apr 15, 2021
@Brandon-G-Tripp
Copy link
Contributor Author

@Tirokk Thanks for the feedback. I will work to get that border taken care of and attempt to write a Cypress test.

I am sorry I did fully understand where to get the email confirmation code from Neo4J to get to the enter password page on the registration slide. I ran the query you suggested and did not receive anything back.

@Tirokk
Copy link
Member

Tirokk commented Apr 19, 2021

Sorry that I gave you the wrong Cypher statement before. @Brandon-G-Tripp
I mixed them up …

@Tirokk
Copy link
Member

Tirokk commented May 3, 2021

Hey @Brandon-G-Tripp !
How is it going?

@Brandon-G-Tripp
Copy link
Contributor Author

@Tirokk I apologize I was out of country for a week and a half and didn't have internet. I should have it finished by the end of the week. I will let you know if I run into any issues with the cypress testing or implementing in the registration slider.

@Tirokk
Copy link
Member

Tirokk commented May 10, 2021

Hey @Brandon-G-Tripp ,
I’m just happy that you have the time to go forward with the development. Nice! Yes, let me know in case you need help with anything.

@ulfgebhardt
Copy link
Member

This branch needs a rebase! @Brandon-G-Tripp
This will then include the required tests. To resolve the yarn.lock conflict one can run yarn install again to let yarn itself resolve the conflict.

@Brandon-G-Tripp
Copy link
Contributor Author

@ulfgebhardt Ok perfect I will rebase. I do not have yarn on my local machine. Is there another way to resolve the conflict or should I install it?

@ulfgebhardt
Copy link
Member

Just do the rebase and if there is a problem i can fix it - no problems <3 ty @Brandon-G-Tripp

@Brandon-G-Tripp Brandon-G-Tripp force-pushed the bgt-show-password-component branch from a91b56a to 654372f Compare May 15, 2021 18:34
Copy link
Member

@ulfgebhardt ulfgebhardt left a comment

Choose a reason for hiding this comment

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

LGTM

@Brandon-G-Tripp
Copy link
Contributor Author

@ulfgebhardt thank you. Looking forward to the next one.

@Tirokk Tirokk changed the title feat: [WIP] 🍰 Show Password Component feat: 🍰 Show Password Component May 18, 2021
@Tirokk Tirokk marked this pull request as ready for review May 18, 2021 19:03
Copy link
Member

@Tirokk Tirokk left a comment

Choose a reason for hiding this comment

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

!!! I did a mistake !!! Had to reset the branch …

Hey @Brandon-G-Tripp 🤗

I don’t know why, but I can't see any show password icons on registration http://localhost:3000/registration at Create user account.

I double checked that I’m on the correct branch. See pic below 👇🏼

Hope I do nothing wrong …
Please check once again your self. 🙏🏼

Bildschirmfoto 2021-05-18 um 20 59 31

Copy link
Member

@Tirokk Tirokk left a comment

Choose a reason for hiding this comment

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

Hey @Brandon-G-Tripp 🤗

Cool and well done, dear !!! 🚀🚀💫💫
Everything works well after I reseted my branch. 🍀

I’m very glad that you cared again for this important feature 🙏🏼

It goes forward with you great help.
So please merge! 😍

PS: The only thing what is not fixed is the double line on the left side, but I really can live with it. If you like you could create a new issue for this.

@Brandon-G-Tripp
Copy link
Contributor Author

@Tirokk Thank you for the review. I am happy to be back working on the project.

I will definitely make an issue for the little css. I had trouble fixing it as the ds-input workaround created it. I will try to resolve it in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants