Skip to content

Conversation

@pallabez
Copy link
Contributor

@pallabez pallabez commented Jan 6, 2024

Issue:

#191

Description:

Anything you would like to inform the reviewer about:

Dev Tested:

  • Yes
  • No

Put the Feature behind Feature Flag

  • Yes
  • No

Test Stats

Images/video of the change:

Follow-up Issues (if any):

NOTE: After your PR is merged in main please verify its working on prod and raise a dev to main PR

@pallabez pallabez self-assigned this Jan 6, 2024
Copy link

@ajoykumardas12 ajoykumardas12 left a comment

Choose a reason for hiding this comment

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

Reviewed with mostly JS knowledge and little Vue knowledge 😅
Also, any visuals for changes?

Comment on lines +42 to +48
user,
token,
setToken,
setUser,
signin,
signout,
isLoading,

Choose a reason for hiding this comment

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

Should token, setToken, setUser be exported? Or they are exported for tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the catch. Will address it.

Comment on lines +55 to +61
const LINKS = [
{ name: 'Welcome', link: 'https://welcome.realdevsquad.com' },
{ name: 'Events', link: 'https://www.realdevsquad.com/events.html' },
{ name: 'Members', link: 'https://members.realdevsquad.com/' },
{ name: 'Crypto', link: 'https://crypto.realdevsquad.com/' },
{ name: 'Status', link: 'https://status.realdevsquad.com/' }
]

Choose a reason for hiding this comment

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

These constants can be moved to constants file.

throw error // or Log here
})
.then(res => transformSelfInfoFromApi(res.data))

Choose a reason for hiding this comment

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

Not catching errors?

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