brigadecore / brigade-dashboard

Apache License 2.0
4 stars 5 forks source link

add global utility function for retrieving user ID #48

Closed DhairyaBahl closed 2 years ago

DhairyaBahl commented 2 years ago

Fixes #17.

Get/store user ID at log in.

krancour commented 2 years ago

@DhairyaBahl looking good. Please do run yarn style:fix on this.

krancour commented 2 years ago

@DhairyaBahl looks like you've amended the PR, but I still see a lot of changes I wasn't expecting in App.tsx.

DhairyaBahl commented 2 years ago

@krancour It would be amazing if you can list out those changes. I will work on those asap.

krancour commented 2 years ago

@DhairyaBahl look at the diffs.

DhairyaBahl commented 2 years ago

@krancour I think now it should be good to go. I've removed all the unwanted changes from App.tsx. I am just filling data in LS onLogin and clearing data from LS on logout. Kindly review this.

DhairyaBahl commented 2 years ago

@krancour Kindly review it again and tell me what more I can improve ? or is it ok ?

PS: I don't know why but I overcomplicated this PR badly. Will take care of that in future. 🙂

krancour commented 2 years ago

PS: I don't know why but I overcomplicated this PR badly. Will take care of that in future.

@DhairyaBahl I don't think it was that bad. I think the direction of the PR pivoted a few times based on our ongoing collaboration. That's to be expected.

We're landing in a good place. No worries!

And thank you for your patience.

DhairyaBahl commented 2 years ago

@krancour I think now it is good to go. 🔥

DhairyaBahl commented 2 years ago

And thank you for your patience.

Thanks to you kent. Thanks for not losing patience when I was make multiple commits with small changes. Other maintainers would have ignored this PR 😂.

krancour commented 2 years ago

This looks great now. The one last, small thing I might suggest is renaming UserLogin.ts to maybe just User.ts since the functionality within is not really coupled to the login process at all anymore, but simply a lazy fetch / cache for the user ID.

DhairyaBahl commented 2 years ago

@krancour To be Honest ! I was using the name User.ts earlier but then linter started giving me some errors. It got confused between the User.ts and User.tsx. Kindly confirm this once.

image
krancour commented 2 years ago

Weird... I don't understand why the linter didn't like that. I will look as soon as I get a chance. This PR is great. Thank you for all the hard work!

DhairyaBahl commented 2 years ago

@krancour Kindly review this one and Merge this if everything if ok so that I can work on other PRs which are dependent on this one. 😄

krancour commented 2 years ago

/brig run

krancour commented 2 years ago

@DhairyaBahl I see what the problem was with User.ts. Since there was also a User.tsx, import statements like import User from "./User" became ambiguous.

I amended this PR with a commit that moves getUserID() into the same file as getClient() and renames it to UItils.ts.

fwiw, I usually consider anything named "utils" to be a "smell" that says, "I just didn't know where else to put this." But this seems like an ok short-term solution.

krancour commented 2 years ago

/brig run

krancour commented 2 years ago

/brig run

krancour commented 2 years ago

/brig run

krancour commented 2 years ago

/brig run

DhairyaBahl commented 2 years ago

@DhairyaBahl I see what the problem was with User.ts. Since there was also a User.tsx, import statements like import User from "./User" became ambiguous.

I amended this PR with a commit that moves getUserID() into the same file as getClient() and renames it to UItils.ts.

fwiw, I usually consider anything named "utils" to be a "smell" that says, "I just didn't know where else to put this." But this seems like an ok short-term solution.

Ohh ! I see ! Understood.

DhairyaBahl commented 2 years ago

@krancour I can see you are facing some problems with this PR. Is there anything I can help you with ?

krancour commented 2 years ago

/brig run

krancour commented 2 years ago

Thanks for all the hard work on this @DhairyaBahl

DhairyaBahl commented 2 years ago

Thanks for all the hard work on this @DhairyaBahl

It was great experience to collaborate with you on this PR. 😄