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

Frontend PR #3

Open
wants to merge 9 commits into
base: main2
Choose a base branch
from
Open

Frontend PR #3

wants to merge 9 commits into from

Conversation

iadnanmalik
Copy link
Owner

No description provided.

@iadnanmalik iadnanmalik requested a review from Lutif August 14, 2021 18:45
Copy link

@Lutif Lutif left a comment

Choose a reason for hiding this comment

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

  • why are some component files .jsx while other .js?
  • move the component folder out of the pages folder.
  • too many extra line spaces and indentation issues.

const UsersContext = createContext();

export function UsersWrapper({ children }) {
const [user,setUser]= useState([])
Copy link

Choose a reason for hiding this comment

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

why is user an array?

padding: 1rem !important;


`
Copy link

Choose a reason for hiding this comment

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

spacing

const [mObj,setmObj]=useState({})
const [messageList,setMessageList]= useState([])
const messagesEndRef = useRef(null)
const context= useUsersContext()
Copy link

Choose a reason for hiding this comment

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

avoid genric naming

const [socketState, setSocketState]=useState()
//console.log("We've Re-rendered")

let socketRef= null
Copy link

Choose a reason for hiding this comment

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

will sockedState and socketRef be always the same?

message:event.target.value,
to:selectedContact,
from:context.user.name,
time:new Date().toLocaleTimeString([],
Copy link

Choose a reason for hiding this comment

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

why not set these params before sending messages instead of setting them again and again on each character change?

</div>
</ContactColumn>

<ChatColumn style={{border:'1px solid black', 'border-radius':'10px'}}>
Copy link

Choose a reason for hiding this comment

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

avoid inline styles

{
messageList?.map( message => {
if(message.from===selectedContact || (message.from === context.user.name && message.to=== selectedContact))
return <div key={dummy()} style={{overflow: 'hidden'}}>
Copy link

Choose a reason for hiding this comment

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

provide valid key

if(user.name !== context.user.name){
return (
<ContactListItem
key={dummy()}
Copy link

Choose a reason for hiding this comment

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

provide valid key

if(message.from===selectedContact || (message.from === context.user.name && message.to=== selectedContact))
return <div key={dummy()} style={{overflow: 'hidden'}}>
{ message.from=== context.user.name ?
<Message style={{float:'right',backgroundColor:'#E1EBEE'}}>{ message.message }<span>{message.time}</span></Message>
Copy link

Choose a reason for hiding this comment

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

no inline styles

</div>
})
}
<div ref={messagesEndRef} />
Copy link

Choose a reason for hiding this comment

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

can you find a better way to get auto-scroll functionality .

@iadnanmalik iadnanmalik requested a review from Lutif August 20, 2021 10:56
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.

2 participants