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 : implemented login and logout functionality #103

Conversation

VishalPawar1010
Copy link
Contributor

Description

This PR fixes #91 : Implemented login and logout functinality to Pieces OS
Here is test recording : https://www.awesomescreenshot.com/video/27665717?key=10fe1fa66bd6c3d3febe8b72935ee5f4

Regarding changes for _indicator.firstElementChild : It was showing following error at runtime, thus updated the code to resolve it.

Uncaught runtime errors:
×
ERROR
Cannot read properties of null (reading 'firstElementChild')
TypeError: Cannot read properties of null (reading 'firstElementChild')
    at http://localhost:3000/static/js/bundle.js:202:18
ERROR
Cannot read properties of null (reading 'firstElementChild')
TypeError: Cannot read properties of null (reading 'firstElementChild')
    at http://localhost:3000/static/js/bundle.js:202:18

@VishalPawar1010 VishalPawar1010 changed the title #91 : implemented login and logout functionality feat : implemented login and logout functionality May 14, 2024
Copy link
Contributor

@Arindam200 Arindam200 left a comment

Choose a reason for hiding this comment

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

Please Remove the unnecessary spaces, the rest looks good

src/app/App.tsx Outdated Show resolved Hide resolved
src/app/App.tsx Outdated Show resolved Hide resolved
src/app/App.tsx Outdated Show resolved Hide resolved
@VishalPawar1010
Copy link
Contributor Author

@Arindam200 I have removed most of the unnecessary spaces.
Could you please review, please let me know for any.
CC : @shivay-at-pieces

Copy link
Contributor

@shivay-at-pieces shivay-at-pieces left a comment

Choose a reason for hiding this comment

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

The overall implementation looks great. But we would like to also share some additional details when the user is logged in.

For instance, when the user logs in, you can show their name at the top. In additional, can we reduce the size of the buttons for login / log out.

In addition, we should have an option to be able to use the example without being logged in. So that option to login should appear in the main screen

@VishalPawar1010
Copy link
Contributor Author

The overall implementation looks great. But we would like to also share some additional details when the user is logged in.

For instance, when the user logs in, you can show their name at the top. In additional, can we reduce the size of the buttons for login / log out.

In addition, we should have an option to be able to use the example without being logged in. So that option to login should appear in the main screen

Sure, will look into below details for this PR

  • Show details of logged in user at the top
  • Reduce the size of the login/logout buttons
  • Make login feature optional

Thanks for the critical review

@shivay-at-pieces
Copy link
Contributor

shivay-at-pieces commented May 28, 2024

Also @VishalPawar1010 right now, if the user is already logged into Pieces, then you should automatically enter the application and it should be able to identify that. That is that the user should only see the Login screen if Pieces OS isn't actually having a logged in state
image

@VishalPawar1010
Copy link
Contributor Author

VishalPawar1010 commented Jun 2, 2024

Noted down the additional points to be considered -

  • Add logic to check if user is already loggen in and if yes - navigate user to application
  • User can see login screen only if user is not in logged-in state.

I will look into this issue once I am done with issue #62 .

@mason-at-pieces
Copy link
Collaborator

@shivay-at-pieces @VishalPawar1010 Is there a reason we are requiring users to authenticate? You can utilize every feature besides things like sharing without authenticating.

Id rather let users use everything without authenticating but they can if they want

@VishalPawar1010
Copy link
Contributor Author

Hey @shivay-at-pieces, as my previous PR is merged, I can move forward for this PR now.

As Mason asked in previous comment, please let me know if we need to continue work on this issue.

Thanks

@VishalPawar1010
Copy link
Contributor Author

VishalPawar1010 commented Jul 17, 2024

Hey @shivay-at-pieces , @Arindam200 , this PR is ready for review.
I have made all the updates mentioned earlier, listing them below -

  • Show details of logged in user at the top : Done -> diplayed loggedIn user name at top
  • Reduce the size of the login/logout buttons : Done
  • Make login feature optional: Done
  • Add logic to check if user is already logged in and if yes - navigate user to application: Done
  • User can see login button only if user is not in logged-in state: Done

Please let me know for any mofifications.
Thanks.

Copy link
Contributor

@shivay-at-pieces shivay-at-pieces left a comment

Choose a reason for hiding this comment

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

This is a much better implementation @VishalPawar1010!
Thanks for the contribution. Just one small request: when the application loads for the first time or even if you refresh, the login button appears for a few seconds and only then you see the user being logged in with the logout button.

To ensure that the application knows whether the user is logged in as soon as it loads or refreshes, you can use the useEffect hook to check the login status immediately upon mounting

@VishalPawar1010
Copy link
Contributor Author

Thank you for the critical review.
I will handle this scenario.

@VishalPawar1010
Copy link
Contributor Author

Handled the mentioned case.
Even using useEffect it was not able to rectify issue, as conditionis depenedend on one api call which takes time.
I implemented onother logic to resolve this.
Test video : https://www.awesomescreenshot.com/video/29867569?key=7f651723ff5ce5e2e414ccadf534058c

@shivay-at-pieces
Copy link
Contributor

Thanks this looks great

@shivay-at-pieces shivay-at-pieces merged commit 973e649 into pieces-app:main Jul 25, 2024
1 check passed
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.

Implement Login and Logout Functionality to Pieces OS
4 participants