Review neu #10

thecactoos · 2023-04-16T20:04:12Z

No description provided.

After log out user couldn't open socket connection without reload.

Fix bug with socket initiation

Left some warnings/errors to know where to eke out unhandled actions

BackToNav was misleading name of the component. Improves readability.

…n, add preview if conversation exists -Add Redirect inside component, depending on newly created conversation -Check existence of DM conversation, if exists show preview, after submit redirect to created/existed conversation -Fix avatar, and heading display

…nut, add clearing data to SelectType.js Clearing data on unmount of NewConveration was causing clearing data on changing route. It was unwanted action, decided to add clear action whenever user hit SelectType route

Add missing functionalities, fix redirect.

…nnecessary comment

… unnecessary comment - Change name isMobile to isSmallScreen, small screen doesn't have to be mobile - Sidebar is rendered conditionally to improve performance

When new message received or created, the newest chats didn't go to first position

The name did not correspond to the performen action

… unnecessary comment

*Change sending animation from keyframes to react-spring *Fix rerender component to display sending animation correctly by wrap component by React.memo *Remove unnused scss lines, change name of svg component

Hook which returns boolean representing is element is visible on screen

Scrolling functionalities: * When conversation show-up, automatically scroll messages to bottom * When new message create or received scroll to bottom * When scroll to top get messages history (it's not finished, waiting for backend)

Make it work after break

Probably temporary fix, but have more important things to do than configuration npm packages

Fix create-react-app app warning asociated to failing to load source map

Probably temporary fix, but have more important things to do than configuration npm packages

* Configure eslint, apply changes coming from linter * Move back to setupProxy.js instead .ts ?

* Add process.env.API_PROD_URL * Add environmental dependency in url's connections * Delete CI env variable in env setup * Add netlify.toml to deploy without 404 etc. * Add build folder to git ignore * Add setupProxy.js and configure proxy for development

* Update socket.io to correct connection with cookies * Update socket.io initiation on files * Fix: Update url in setupProxy.js, because it's work only in dev mode

Add typescript and prepare to deploy

moonith

Few more comments:

I don't think redux is required for an app of this complexity - read more on composition https://felixgerschau.com/react-component-composition/
You are using custom hooks as kind of controller for react views, its an overkill
TS is lacking here
No tests

moonith · 2023-04-16T20:11:01Z

package.json

+  "private": true,
+  "homepage": "https://chatark.netlify.app/",
+  "dependencies": {
+    "@testing-library/jest-dom": "^4.2.4",


its dev dependency, in general its not that important what section it's in but sometime tools use this distinction to optimise end package size

moonith · 2023-04-16T20:16:14Z

public/manifest.json

@@ -0,0 +1,15 @@
+{


its important to keep app metadata updated

moonith · 2023-04-16T20:17:56Z

src/App.test.js

@@ -0,0 +1,10 @@
+/* eslint-disable no-undef */


you should disable eslint rules, but sometimes if 3rd party lib code is broken we have to do that. In such a case one should disable eslint for certain line, not whole file

moonith · 2023-04-16T20:21:01Z

src/App.test.js

+import { render } from '@testing-library/react';
+import App from './pages/App';
+
+test('renders learn react link', () => {


when writing tests you should always answer this question - is this test helpful? I know this is an example test from react create app, but its a good place to talk about this. In this case this test does not make sense because app.js contained this link in jsx without any conditional logic

moonith · 2023-04-16T20:23:37Z

src/actions/auth-action-types.js

@@ -0,0 +1,15 @@
+// Signing in
+export const SIGN_IN_SUCCESS = 'SIGN_IN_SUCCESS';
+export const SIGN_IN_FAIL = 'SIGN_IN_FAIL';


should be SIGN_IN_FAILURE, same everywhere else

moonith · 2023-04-18T19:15:28Z

src/pages/Navigation/Users/User/User.js

+      <Link
+        to={{
+          pathname: `${PROFILE_USER_WITHOUT_ID}${id}`,
+          state: { from: location.pathname },


what is the purpose of storing origination path in route state? I saw only that you use it to mimic browser back button - there are built in methods for that

moonith · 2023-04-18T19:17:37Z

src/pages/Navigation/useNavigation.js

+    }
+  }, [searchUsers, searchValue]);
+
+  useEffect(() => {


you have two use effects that are recalculating based on same searchValue prop. Check https://blog.battlefy.com/how-to-escape-react-hooks-hell-a66c0d142c9e

moonith · 2023-04-18T19:19:50Z

src/pages/NewConversation/DirectMessageForm/useDirectMessageForm.js

+      return false;
+    }
+    const membersIds = [
+      ...new Set([...receivers.map((receiver) => receiver.id), userId]),


Why set here converted to array, are you trying to commit duplicates in array that way?

moonith · 2023-04-18T19:27:07Z

src/pages/NewConversation/GroupInfoForm/GroupInfoForm.js

+
+  if (createdConversation) {
+    return (
+      <Redirect


This is a bizarre way of doing redirects. You should opt out from putting such a business logic into jsx

moonith · 2023-04-18T19:47:44Z

src/pages/NewConversation/SearchReceiversInputDesktop/SearchReceiversInputDesktop.js

+        type="text"
+        value={searchValue}
+        onChange={(e) => {
+          handleSearchChange(e);


I don't understand this separation here

thecactoos and others added 30 commits February 25, 2021 10:10

Add client to GIT respository

4545073

Fix bug with socket initiation

aea3aaa

After log out user couldn't open socket connection without reload.

Merge pull request #1 from thecactoos/fix-socket-connetion

425001b

Fix bug with socket initiation

Add eslintrc.json file and remove most of the eslint errors

19ccca7

Left some warnings/errors to know where to eke out unhandled actions

Change name of the component from BackToNav to BackLink

ecb3dc2

BackToNav was misleading name of the component. Improves readability.

Add missing import description

3d78457

Update Readme

b4e8efa

Add reselect package

8c2189f

Add action to remove receivers

9d5ea98

Change name of destructured variable, to correctly display avatar

82f3cc1

Add Redirect inside component, depending on newly created conversation

a8d6aa8

Merge pull request #2 from thecactoos/fix-new-conversation-redirect

c8c3d8f

Add missing functionalities, fix redirect.

Change animation when redirect is from DirectMessageForm.js, remove u…

126ecc7

…nnecessary comment

Fix redirect after conversation creation

92ad80d

Change display Sidebar conditionally, change name of variable, delete…

e06c924

… unnecessary comment - Change name isMobile to isSmallScreen, small screen doesn't have to be mobile - Sidebar is rendered conditionally to improve performance

Fix overflowing messages

8ba797b

Fix displaying chats by lastMessageDate

d55ecda

When new message received or created, the newest chats didn't go to first position

Fix name of saga

cc2ad81

The name did not correspond to the performen action

Add withCredential to axios config, fix path in logout action, remove…

8120d3d

… unnecessary comment

Change sending animation, fix rerender, clean-up

3eb3b85

*Change sending animation from keyframes to react-spring *Fix rerender component to display sending animation correctly by wrap component by React.memo *Remove unnused scss lines, change name of svg component

Create useOnScreen hooks

4893856

Hook which returns boolean representing is element is visible on screen

Make it work after break

fb11b95

Add todo.txt to git ignore

5337467

Merge pull request #3 from thecactoos/make-it-work-after-break

fdf5268

Make it work after break

Fix create-react-app app warning asociated to failing to load source map

70a4ae0

Probably temporary fix, but have more important things to do than configuration npm packages

Merge pull request #4 from thecactoos/make-it-work-after-break

c30c142

Fix create-react-app app warning asociated to failing to load source map

Fix create-react-app app warning asociated to failing to load source map

8807475

Probably temporary fix, but have more important things to do than configuration npm packages

thecactoos and others added 10 commits April 12, 2023 03:44

Add Typescript to project

4f5c4a8

* Configure eslint, apply changes coming from linter * Move back to setupProxy.js instead .ts ?

Prepare to deploy

f2bd635

* Add process.env.API_PROD_URL * Add environmental dependency in url's connections * Delete CI env variable in env setup * Add netlify.toml to deploy without 404 etc. * Add build folder to git ignore * Add setupProxy.js and configure proxy for development

Update socket.io

27a3004

* Update socket.io to correct connection with cookies * Update socket.io initiation on files * Fix: Update url in setupProxy.js, because it's work only in dev mode

Fix: Change path in socket.io

567d448

Fix: Put a logout call to try catch

af96f91

Fix logout bug

63e7ff4

Fix: Eliminate wrong redirect

10314d3

Merge pull request #5 from thecactoos/ts-configuration

8f3c47f

Add typescript and prepare to deploy

Update links

3315b18

Merge branch 'empty2' into review_neu

afd7577

thecactoos requested a review from moonith April 16, 2023 20:04

moonith reviewed Apr 18, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Review neu #10

Review neu #10

Uh oh!

thecactoos commented Apr 16, 2023

Uh oh!

moonith left a comment

Uh oh!

moonith Apr 16, 2023

Uh oh!

moonith Apr 16, 2023

Uh oh!

moonith Apr 16, 2023

Uh oh!

moonith Apr 16, 2023

Uh oh!

moonith Apr 16, 2023

Uh oh!

moonith Apr 18, 2023

Uh oh!

moonith Apr 18, 2023

Uh oh!

moonith Apr 18, 2023

Uh oh!

moonith Apr 18, 2023

Uh oh!

moonith Apr 18, 2023

Uh oh!

Uh oh!

Review neu #10

Are you sure you want to change the base?

Review neu #10

Uh oh!

Conversation

thecactoos commented Apr 16, 2023

Uh oh!

moonith left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!