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

Global signin #35

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Global signin #35

wants to merge 18 commits into from

Conversation

RhythmAgg
Copy link
Collaborator

Global Signin

Comment on lines 60 to 62
const GET_URL_DEV =
'https://datafoundation.iiit.ac.in/api/detokn?token=' + tokenId;
// const GET_URL_DEV = 'http://10.4.25.20:3001/api/detokn?token=' + tokenId;
Copy link
Collaborator

Choose a reason for hiding this comment

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

move this to constants

Copy link
Collaborator

Choose a reason for hiding this comment

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

Capital case is only used when it is a global constant

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

resolved

Comment on lines 60 to 62
const GET_URL_DEV =
'https://datafoundation.iiit.ac.in/api/detokn?token=' + tokenId;
// const GET_URL_DEV = 'http://10.4.25.20:3001/api/detokn?token=' + tokenId;
Copy link
Collaborator

Choose a reason for hiding this comment

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

pls remove comments in PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

resolved

if (isLoggedIn) {
return <Navigate replace to="/" />;
} else {
// <Login checkUser={checkUser} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove comments

Copy link
Collaborator

Choose a reason for hiding this comment

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

  • login component is not needed. Delete the file

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

resplved

// <Login checkUser={checkUser} />
window.location.href =
'https://datafoundation.iiit.ac.in/sign-in?redirect=/hv';
return <p>Redirecting ...</p>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this required?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This redirects to login url

Copy link
Collaborator

Choose a reason for hiding this comment

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

config files are not supposed to be pushed. Had pointed out this thing in your previous PR also, if this gets merged, we would have to change the file in server manually.

Copy link
Collaborator

Choose a reason for hiding this comment

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

config is just for development purpose

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@shaantanu314
Copy link
Collaborator

@RhythmAgg Add video in PR

  1. Showing the redirection properly and data getting stored in local storage
  2. Copy paste the token/json obj in localhost local-storage and verify if user stays signed in

.gitignore Outdated
node_modules
client/src/components/Config
Copy link
Collaborator

Choose a reason for hiding this comment

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

add .env also here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@RhythmAgg
Copy link
Collaborator Author

global-signin-demo.mp4

Attached is the demo video for the global signin

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.

3 participants