Closed cch5ng closed 4 years ago
also I believe the FE structure should be a little more close to this (authenticated app and unauthenticated app) but I don't fully understand the login in the react context:
i.e. what is getUser() supposed to do in real life? is it checking for the jwt in session storage? or is it checking whether the stored jwt is authenticated against the BE. it is a little more complex than I've done before so it was not included.
hi, I commented out so many items. at first I was going to try to refactor before making the pr, but I was getting too many console errors to view what was there in the browser.
eventually I decided to remove pointers to the existing components and save the refactor for another task. I just did not uncomment everything that was causing errors.
let me know what you think about the sandbox code. (Kent Dodds example) I think it makes sense but I'm not sure about some of the react context code so I didn't want to throw it in and leave you to figure it out. I think I will try to play with his sample a little this weekend to understand even if we don't use it.
I hope you don't mind I put in those login and signup stubs. it was done before today just to explore material UI (before auth was assigned). and I wanted to test to see if my containers worked for your parts. I guess it depends on how you decide to go for that side image but some bits are reusable. have a good weekend!
are you going to work on the weekend? do you want this merged today?
On Fri, May 22, 2020 at 5:35 PM Rajat Bansal notifications@github.com wrote:
@rjtbansal approved this pull request.
Just some clarifying comments. Looks very useful to me so far. Regarding nested components, use whatever makes sense for you. :)
In client/src/components/Chat/Chat.js https://github.com/hatchways/team-penguin/pull/35#discussion_r429496459:
- // sm={9}
- // direction='row'
- // >
- /* <ChatHeader
- selected={props.selected}
- /> */
- /* <MessageDisplay
- username={props.username}
- messages={props.messages}
- />
- <MessageInput
- username={props.username}
- sendMessage={props.sendMessage}
- /> */
- //
Do we really need this commented out code here? Or you feel its something that could be used in future?
In client/src/components/Chat/ChatHeader.js https://github.com/hatchways/team-penguin/pull/35#discussion_r429496583:
@@ -14,7 +14,7 @@ const ChatHeader = props => ( container spacing={3} alignItems='center'
- className={props.classes.root}
- //className={props.classes.root}
Same query as to why it has been commented for now?
In client/src/components/Login/Login.js https://github.com/hatchways/team-penguin/pull/35#discussion_r429496767:
- {"Don't have an account? Sign Up"}
Thanks for easing my work :)
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/hatchways/team-penguin/pull/35#pullrequestreview-417232959, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABEI72EJ7XWV3LOHPUJ7ECLRS4K5XANCNFSM4NIFMSIA .
Hi Carol, I appreciate all the work that you have done so far and dont worry about login and signup stubs. In fact its helpful for me as well :). I can reuse most of it. Regarding context in React, I myself have never used it so do whatever you think makes sense. I will look into it too. I do plan to work on it over the weekend (not tonight though) so feel free to merge whatever you have end of your day :). Have a good weekend too.
hi, I will merge what is there right now.
I will try to get a better understanding on the react context example. if so, maybe there will be some refactoring on Mon for it. but at least this way, I'm not holding you back for your part and you can test.
I'm going to leave in the comments for now. I know it is bad but I am kind of tired. I will go through it and try to get the other other parts working. thanks for your thoughts.
On Fri, May 22, 2020 at 5:50 PM Rajat Bansal notifications@github.com wrote:
Hi Carol, I appreciate all the work that you have done so far and dont worry about login and signup stubs. In fact its helpful for me as well :). I can reuse most of it. Regarding context in React, I myself have never used it so do whatever you think makes sense. I will look into it too. I do plan to work on it over the weekend (not tonight though) so feel free to merge whatever you have end of your day :). Have a good weekend too.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/hatchways/team-penguin/pull/35#issuecomment-632957630, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABEI72HQP46PLUCMWTNM2V3RS4MVRANCNFSM4NIFMSIA .
it is in a state where Rajat could do FE work.
I was trying to separate the Frontend into two sides, Authenticated and Unauthenticated but I'm not sure of the logic to determine which side using react context. so right now, there are a few routes:
I put in some stub code for Login and Signup components, it is just using MaterialUI components I was practicing with last week. there is not much interaction logic I think in there. you can use what is helpful.
in case you choose to put a static image on the left side, there is this container I had created for that, UnauthenticatedApp (with appropriate grid). otherwise MaterialUI's Login component has a rotating image already there.
for my sections, I will probably keep some high level folders and nest components a little bit. bc otherwise I think components will get overly cluttered. but right now route level components have a flat hierarchy.
Lakshmi's code will be refactored separately.