Refactor chat component #10494
Refactor chat component #10494
Conversation
pr-triage
bot
added
the
PR: draft
|
Hey @narender2031 ! Saw "refactor chat component", and it caught my attention. I just fixed a bug with the chat component causing it to not clear the input after being sent. It's over at #10487. I think it would be compatible with your work if you want to take it into consideration while refactoring, especially the stuff I did with the Compose and Chat components, which cleans up the chat textarea's input state management. I also added tests for the textarea being cleared after the message being sent, which can help you make sure nothing went wrong with the merge. |
| import { h } from 'preact'; | ||
| import PropTypes from 'prop-types'; | ||
|
|
||
| const OpenInroMessage = ({ activeChannel }) => { |
There was a problem hiding this comment.
This is a typo, it should be Intro, not Inro.
| import { h } from 'preact'; | ||
| import PropTypes from 'prop-types'; | ||
|
|
||
| const DireactIntroMessages = ({ activeChannel }) => { |
There was a problem hiding this comment.
This is a typo: Direct, not Direact.
There was a problem hiding this comment.
@narender2031 the draft is starting to look good. I have some feedback and request changes. I'll continue reviewing in the morning.
| triggerDeleteMessage, | ||
| }) => { | ||
| if (!messages[activeChannelId]) { | ||
| return ''; |
There was a problem hiding this comment.
refactor (best practice): There is no need to return an empty string. If a component ends up returning nothing via conditional rendering, return null.
|
|
||
| Message.propTypes = { | ||
| activeChannelId: PropTypes.number, | ||
| messages: PropTypes.arrayOf(PropTypes.object), |
There was a problem hiding this comment.
refactor (blocking): Use PropTypes.shape to describe the shape of a message prop. If the message proptype exist elsewhere, consider importing a shred prop type. For an example of this, see https://github.com/forem/forem/blob/master/app/javascript/common-prop-types/article-prop-types.js.
For more information on PropTypes.shape, see the PropTypes documentation.
| return ( | ||
| <div className="chatmessage" style={{ color: 'grey' }}> | ||
| <div className="chatmessage__body"> | ||
| You have joined {activeChannel.channel_name}! All interactions{' '} |
There was a problem hiding this comment.
refactor (non-blocking): Consider using a template string instead.
| You have joined {activeChannel.channel_name}! All interactions{' '} | |
| {`You have joined ${activeChannel.channel_name}! All interactions `} |
| .then(successCb) | ||
| .catch(failureCb); | ||
| } | ||
| // export function getAllMessages(channelId, messageOffset, successCb, failureCb) { |
There was a problem hiding this comment.
refactor (blocking): Remove dead commented out code.
| @@ -56,20 +57,30 @@ export function editMessage(editedMessage, successCb, failureCb) { | |||
| .catch(failureCb); | |||
| } | |||
|
|
|||
| export function sendOpen(activeChannelId, successCb, failureCb) { | |||
| fetch(`/chat_channels/${activeChannelId}/open`, { | |||
| // export function sendOpen(activeChannelId, successCb, failureCb) { | |||
There was a problem hiding this comment.
refactor (blocking): remove dead commented out code.
| .then((response) => response.json()) | ||
| .then(successCb) | ||
| .catch(failureCb); | ||
| body: JSON.stringify({}), |
There was a problem hiding this comment.
The request utility function converts the body automatically to JSON. See https://github.com/forem/forem/blob/master/app/javascript/utilities/http/request.js#L35
| body: JSON.stringify({}), | |
| body: {}, |
| }); | ||
|
|
||
| return response.json() | ||
| // fetch('/chat_channels?state=unopened_ids', { |
There was a problem hiding this comment.
refactor (blocking): remove dead commented out code.
| // } | ||
|
|
||
| export async function getContent(url) { | ||
| const response = await request(url, { |
There was a problem hiding this comment.
'GET' is the default and credentials: 'same-origin' are included automatically when using the request utility function. See lines https://github.com/forem/forem/blob/master/app/javascript/utilities/http/request.js#L27 and https://github.com/forem/forem/blob/master/app/javascript/utilities/http/request.js#L47
| }) | ||
|
|
||
| return response.json(); | ||
| // fetch(`/chat_channels/${channelId}?message_offset=${messageOffset}`, { |
There was a problem hiding this comment.
refactor (blocking): remove dead commented out code.
|
|
||
| export async function getAllMessages(channelId, messageOffset) { | ||
| const response = await request(`/chat_channels/${channelId}?message_offset=${messageOffset}`, { | ||
| method: 'GET', |
There was a problem hiding this comment.
'GET' is the default and credentials: 'same-origin' are included automatically when using the request utility function. See lines https://github.com/forem/forem/blob/master/app/javascript/utilities/http/request.js#L27 and https://github.com/forem/forem/blob/master/app/javascript/utilities/http/request.js#L47
There was a problem hiding this comment.
refactor (non-blocking): All components and variables defined with an arrow function are unnecessary since there is no use of this. Prefer using a traditional function. This is something I need to update in our frontend documentation and potentially incorporate a lint rule.
For more information about this see the official arrow function documentation on MDN as well as this post, When (and why) you should use ES6 arrow functions — and when you shouldn’t.
| @@ -0,0 +1,307 @@ | |||
| export function connectReducer(state, action) { | |||
There was a problem hiding this comment.
documentation (blocking): For any functions, constants etc. that are exported in an ES module, provide documentation using JSDoc comments. For an example, see https://github.com/forem/forem/blob/master/app/javascript/utilities/http/request.js#L1.
| @@ -0,0 +1,22 @@ | |||
| import { h, createContext } from 'preact'; | |||
There was a problem hiding this comment.
@narender2031, @citizen428, since we're going to go with useReducer and context, I'll merge my PR, #9799, first.
|
@narender2031, @citizen428, once this PR is out of draft mode, I will give it another review. |
pr-triage
bot
added
PR: unreviewed
| import { h } from 'preact'; | ||
| import PropTypes from 'prop-types'; | ||
|
|
||
| const ActiveChannel = ({ _activeChannel }) => { |
There was a problem hiding this comment.
refactor (blocking): This component appears to do only render an empty <div />. Is this component still a work in progress? It also does not appear to be used anywhere in the codebase.
| ? Object.values(channelUsers[activeChannelId]) | ||
| .filter((user) => user.username.match(filterRegx)) | ||
| .map((user) => ( | ||
| <div |
There was a problem hiding this comment.
refactor (blocking): Use a semantic HTML button instead of a div with a role attribute of "button". Since we are in Prreact land, use our design system Button component.
| alt={user.name} | ||
| style={!user.profile_image ? { display: 'none' } : ' '} | ||
| /> | ||
| <span |
There was a problem hiding this comment.
refactor (blocking): Avoid inline styles. Use our CSS utility classes or a custom CSS class if necessary.
app/javascript/chat/ActiveChatChannel/ActiveChannelMemberList.js
Outdated
Show resolved
Hide resolved
| <div className="crayons-modal__box__body"> | ||
| <h3>Are you sure, you want to delete this message?</h3> | ||
| <div className="delete-actions__container"> | ||
| <div |
There was a problem hiding this comment.
refactor (blocking): Use a semantic HTML button instead of a div with a role attribute of "button". Since we are in PRreact land, use the Button component.
| @@ -1,7 +1,7 @@ | |||
| import { h } from 'preact'; | |||
| import PropTypes from 'prop-types'; | |||
|
|
|||
| const ModFaqSection = ({ email, currentMembershipRole }) => { | |||
| const ModFaqSection = ({ currentMembershipRole }) => { | |||
There was a problem hiding this comment.
nitpick (non-blocking): This can just be membershipRole
|
|
||
| const ConnectStateProvider = ({ children, initialState, reducer }) => { |
There was a problem hiding this comment.
This change isn't necessary. It's fine that it uses the function keyword instead of an arrow function. Arrow functions work well when you need scope, i.e. "this".
| @@ -0,0 +1,320 @@ | |||
| import * as Type from './components/ChatTypes'; | |||
| export function connectReducer(state, action) { | |||
| const { type, payload = {} } = action; | |||
There was a problem hiding this comment.
refactor (blocking): There is no need to give payload a default value of {}. This will actually mask potential errors when a payload isn't being sent that may be required by an action.
| export function connectReducer(state, action) { | ||
| const { type, payload = {} } = action; | ||
|
|
||
| switch (type) { |
There was a problem hiding this comment.
refactor (non-blocking): consider combining reducers instead of a super long switch case.
There was a problem hiding this comment.
I will do these changes incoming PRs with more refactoring.
pr-triage
bot
added
PR: reviewed-changes-requested
Co-authored-by: Nick Taylor <nick@iamdeveloper.com>
…eactIntroMessages.js Co-authored-by: Nick Taylor <nick@iamdeveloper.com>
| @@ -0,0 +1,23 @@ | |||
| require "rails_helper" | |||
|
|
|||
| RSpec.describe "Admin creates new page", type: :system do | |||
There was a problem hiding this comment.
I'm a bit confused, is this spec really related to the changes in this PR?
There was a problem hiding this comment.
these changes updated due to conflict. I think this file is deleted in the master branch. I will check and update this.
| @@ -1,6 +1,31 @@ | |||
| import { h } from 'preact'; | |||
| import PropTypes from 'prop-types'; | |||
|
|
|||
| /** | |||
| * This component is used to render the list of all Achtive chat channel Members | |||
There was a problem hiding this comment.
spelling (nitpick)
| * This component is used to render the list of all Achtive chat channel Members | |
| * This component is used to render the list of all Active chat channel Members |
| @@ -3,7 +3,36 @@ import PropTypes from 'prop-types'; | |||
| import ActionMessage from '../actionMessage'; | |||
| import Message from '../message'; | |||
| import DirectChatInfoMessage from './IntroductionMessages/DireactIntroMessages'; | |||
| import OpenChatInfoMessagge from './IntroductionMessages/OpenItnroMessage'; | |||
| import IntroductionMessage from './IntroductionMessages/OpenItnroMessage'; | |||
There was a problem hiding this comment.
spelling (nitpick): OpenItnroMessage should be OpenIntroMessage
| import IntroductionMessage from './IntroductionMessages/OpenItnroMessage'; | |
| import IntroductionMessage from './IntroductionMessages/OpenIntroMessage'; |
There was a problem hiding this comment.
I think all the feedback has been addressed now, thanks @narender2031! 🚀
pr-triage
bot
added
PR: partially-approved
@citizen428 & @nickytonline I need a small help Can you please test these changes locally. Just for a safe case. |
|
@narender2031 Could you please fill in the "QA Instructions" section of the PR to point out what to look for specifically? This also helps other potential reviewers who weren't following along. |
QA Instructions:-
|
|
Quick question: since this is a refactoring PR, are you planning on also addressing some of the CodeClimate issues or will you do that separately? |
we will do separately because some of the files have a large number of the line so we will refactor those also one by one. |
|
Feedback:
|
@citizen428 You need to integrate the PUSHER locally for this. Can you please check with the other branch? is that happening with all. Because in my case locally in other branches messages don't show up on send or updates. It requires refresh. |
|
Yes @narender2031, correct re 3, I just realized this after changing branches and noticing the same behavior. Sorry for the false alarm on this one. |
np @citizen428 |
pr-triage
bot
added
PR: unreviewed
@citizen428 issues are fixed. |
|
@narender2031 and @sarthology, can we pause work on this PR until #11332 and #9741 are finished? There are change requests in both of those PRs. cc: @citizen428 |
|
As discussed, going to put this on hold and reassess the refactor. Closing this for now. |
What type of PR is this? (check all applicable)
Description
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Please replace this line with instructions on how to test your changes, as well
as any relevant images for UI changes.
Added tests?
Added to documentation?
[optional] Are there any post deployment tasks we need to perform?
[optional] What gif best describes this PR or how it makes you feel?