From b701ba9c0d568a7d82fa6a1a5a205401ecd60534 Mon Sep 17 00:00:00 2001 From: Simon Backx Date: Tue, 30 Aug 2022 16:25:40 +0200 Subject: [PATCH] Cleaned up Comment component (#8) refs https://github.com/TryGhost/Team/issues/1858 fixes https://github.com/TryGhost/Team/issues/1789 - Split up the Comment component in many small Components to make it easier to read and maintain - Added support for synchronous actions, which are required for actions that affect the context state based on the current value (e.g., increasing a value by one) because of the asynchronous nature of setState: Before this change ``` // Context state: {hello: 0}; dispatchAction('increaseHelloByOne') dispatchAction('increaseHelloByOne') ``` Could end up having a state `{hello: 1}` instead of the expected `{hello: 2}`, because underlying this would resolve into: ``` // Context state: {hello: 0}; const hello = {hello: 0}; setState({hello: hello + 1}); setState({hello: hello + 1}); ``` Instead of ``` // Context state: {hello: 0}; setState(({hello}) => {hello: hello + 1}); setState(({hello}) => {hello: hello + 1}); ``` Synchronous actions now support this. - Removed deprecated `onAction` context state function - Replaced the boolean based form checking by the more reliable counter based checking that uses synchronous actions (reason we needed synchronous actions) (fixes https://github.com/TryGhost/Team/issues/1789) - Prevent creating a new `dispatchAction` function every time the context state changes, by using bind. This prevents infinte updates in `useEffect` hooks that depend on `dispatchAction` and also update the context via actions. --- apps/comments-ui/src/App.js | 29 +- apps/comments-ui/src/AppContext.js | 11 +- apps/comments-ui/src/actions.js | 40 ++- apps/comments-ui/src/components/Avatar.js | 42 ++- apps/comments-ui/src/components/Comment.js | 333 ++++++++++++------ .../comments-ui/src/components/CommentsBox.js | 14 +- apps/comments-ui/src/components/Form.js | 18 +- apps/comments-ui/src/utils/helpers.js | 4 + 8 files changed, 331 insertions(+), 160 deletions(-) diff --git a/apps/comments-ui/src/App.js b/apps/comments-ui/src/App.js index b7385043cd..7e1140db53 100644 --- a/apps/comments-ui/src/App.js +++ b/apps/comments-ui/src/App.js @@ -1,6 +1,6 @@ import Frame from './components/Frame'; import React from 'react'; -import ActionHandler from './actions'; +import {isSyncAction, ActionHandler, SyncActionHandler} from './actions'; import {createPopupNotification} from './utils/helpers'; import AppContext from './AppContext'; import {hasMode} from './utils/check-mode'; @@ -51,10 +51,14 @@ export default class App extends React.Component { customSiteUrl: props.customSiteUrl, postId: props.postId, popup: null, - accentColor: props.accentColor + accentColor: props.accentColor, + secundaryFormCount: 0 }; this.adminApi = null; this.GhostApi = null; + + // Bind to make sure we have a variable reference (and don't need to create a new binded function in our context value every time the state changes) + this.dispatchAction = this.dispatchAction.bind(this); } /** Initialize comments setup on load, fetch data and setup state*/ @@ -111,6 +115,15 @@ export default class App extends React.Component { /** Handle actions from across App and update App state */ async dispatchAction(action, data) { + if (isSyncAction(action)) { + // Makes sure we correctly handle the old state + // because updates to state may be asynchronous + // so calling dispatchAction('counterUp') multiple times, may yield unexpected results if we don't use a callback function + this.setState((state) => { + return SyncActionHandler({action, data, state, api: this.GhostApi, adminApi: this.adminApi}); + }); + return; + } clearTimeout(this.timeoutId); this.setState({ action: `${action}:running` @@ -252,7 +265,7 @@ export default class App extends React.Component { /**Get final App level context from App state*/ getContextFromState() { - const {action, popupNotification, customSiteUrl, member, comments, pagination, commentCount, postId, admin, popup} = this.state; + const {action, popupNotification, customSiteUrl, member, comments, pagination, commentCount, postId, admin, popup, secundaryFormCount} = this.state; return { action, popupNotification, @@ -272,14 +285,12 @@ export default class App extends React.Component { appVersion: this.props.appVersion, stylesUrl: this.props.stylesUrl, publication: this.props.publication, + secundaryFormCount: secundaryFormCount, popup, - dispatchAction: (_action, data) => this.dispatchAction(_action, data), - /** - * @deprecated - * Use dispatchAction instead - */ - onAction: (_action, data) => this.dispatchAction(_action, data) + // Warning: make sure we pass a variable here (binded in constructor), because if we create a new function here, it will also change when anything in the state changes + // causing loops in useEffect hooks that depend on dispatchAction + dispatchAction: this.dispatchAction }; } diff --git a/apps/comments-ui/src/AppContext.js b/apps/comments-ui/src/AppContext.js index 785b5f452d..8a1f73897a 100644 --- a/apps/comments-ui/src/AppContext.js +++ b/apps/comments-ui/src/AppContext.js @@ -1,15 +1,6 @@ // Ref: https://reactjs.org/docs/context.html const React = require('react'); -const AppContext = React.createContext({ - site: {}, - member: {}, - action: '', - brandColor: '', - pageData: {}, - onAction: (action, data) => { - return {action, data}; - } -}); +const AppContext = React.createContext({}); export default AppContext; diff --git a/apps/comments-ui/src/actions.js b/apps/comments-ui/src/actions.js index 8cb8e0bf44..b3b84e3761 100644 --- a/apps/comments-ui/src/actions.js +++ b/apps/comments-ui/src/actions.js @@ -332,6 +332,26 @@ function closePopup() { }; } +function increaseSecundaryFormCount({state}) { + return { + secundaryFormCount: state.secundaryFormCount + 1 + }; +} + +function decreaseSecundaryFormCount({state}) { + return { + secundaryFormCount: state.secundaryFormCount - 1 + }; +} + +// Sync actions make use of setState((currentState) => newState), to avoid 'race' conditions +const SyncActions = { + openPopup, + closePopup, + increaseSecundaryFormCount, + decreaseSecundaryFormCount +}; + const Actions = { // Put your actions here addComment, @@ -345,16 +365,28 @@ const Actions = { addReply, loadMoreComments, loadMoreReplies, - updateMember, - openPopup, - closePopup + updateMember }; +export function isSyncAction(action) { + return !!SyncActions[action]; +} + /** Handle actions in the App, returns updated state */ -export default async function ActionHandler({action, data, state, api, adminApi}) { +export async function ActionHandler({action, data, state, api, adminApi}) { const handler = Actions[action]; if (handler) { return await handler({data, state, api, adminApi}) || {}; } return {}; } + +/** Handle actions in the App, returns updated state */ +export function SyncActionHandler({action, data, state, api, adminApi}) { + const handler = SyncActions[action]; + if (handler) { + // Do not await here + return handler({data, state, api, adminApi}) || {}; + } + return {}; +} diff --git a/apps/comments-ui/src/components/Avatar.js b/apps/comments-ui/src/components/Avatar.js index 7405578043..e421c7864a 100644 --- a/apps/comments-ui/src/components/Avatar.js +++ b/apps/comments-ui/src/components/Avatar.js @@ -3,9 +3,23 @@ import AppContext from '../AppContext'; import {getInitials} from '../utils/helpers'; import {ReactComponent as AvatarIcon} from '../images/icons/avatar.svg'; -const Avatar = (props) => { - const {member} = useContext(AppContext); - const memberName = member?.name ?? props.comment?.member?.name; +const Avatar = ({comment, size, isBlank}) => { + const {member, avatarSaturation} = useContext(AppContext); + const dimensionClasses = (size === 'small' ? 'w-6 h-6 sm:w-8 sm:h-8' : 'w-9 h-9 sm:w-[40px] sm:h-[40px]'); + + // When an avatar has been deleted or hidden + // todo: move to seperate component + if (isBlank) { + return ( +
+
+ +
+
+ ); + } + + const memberName = member?.name ?? comment?.member?.name; const getHashOfString = (str) => { let hash = 0; @@ -21,13 +35,13 @@ const Avatar = (props) => { }; const generateHSL = () => { - let commentMember = (props.comment ? props.comment.member : member); + let commentMember = (comment ? comment.member : member); if (!commentMember || !commentMember.name) { return [0,0,10]; } - const saturation = isNaN(props.saturation) ? 50 : props.saturation; + const saturation = isNaN(avatarSaturation) ? 50 : avatarSaturation; const hRange = [0, 360]; const lRangeTop = Math.round(saturation / (100 / 30)) + 30; @@ -46,11 +60,11 @@ const Avatar = (props) => { }; const commentGetInitials = () => { - if (props.comment && !props.comment.member) { + if (comment && !comment.member) { return getInitials('Deleted member'); } - let commentMember = (props.comment ? props.comment.member : member); + let commentMember = (comment ? comment.member : member); if (!commentMember || !commentMember.name) { return getInitials('Anonymous'); @@ -58,9 +72,8 @@ const Avatar = (props) => { return getInitials(commentMember.name); }; - let dimensionClasses = (props.size === 'small' ? 'w-6 h-6 sm:w-8 sm:h-8' : 'w-9 h-9 sm:w-[40px] sm:h-[40px]'); - let initialsClasses = (props.size === 'small' ? 'text-sm' : 'text-lg'); - let commentMember = (props.comment ? props.comment.member : member); + let initialsClasses = (size === 'small' ? 'text-sm' : 'text-lg'); + let commentMember = (comment ? comment.member : member); const bgColor = HSLtoString(generateHSL()); const avatarStyle = { @@ -80,15 +93,6 @@ const Avatar = (props) => { ); - // When an avatar has been deleted or hidden - if (props.isBlank) { - avatarEl = ( -
- -
- ); - } - return (
{avatarEl} diff --git a/apps/comments-ui/src/components/Comment.js b/apps/comments-ui/src/components/Comment.js index e6da7ab8ec..76c2f66e62 100644 --- a/apps/comments-ui/src/components/Comment.js +++ b/apps/comments-ui/src/components/Comment.js @@ -1,4 +1,4 @@ -import React, {useContext, useEffect, useState} from 'react'; +import React, {useContext, useState} from 'react'; import {Transition} from '@headlessui/react'; import Avatar from './Avatar'; import Like from './Like'; @@ -7,39 +7,58 @@ import More from './More'; import Form from './Form'; import Replies from './Replies'; import AppContext from '../AppContext'; -import {formatRelativeTime, formatExplicitTime} from '../utils/helpers'; +import {formatRelativeTime, formatExplicitTime, isCommentPublished} from '../utils/helpers'; -function EditedInfo({comment}) { - if (!comment.edited_at) { - return null; - } +function AnimatedComment({comment, parent}) { return ( - - ·Edited - + + + ); } -const Comment = ({updateIsEditing = null, isEditing, ...props}) => { +function EditableComment({comment, parent}) { const [isInEditMode, setIsInEditMode] = useState(false); - const [isInReplyMode, setIsInReplyMode] = useState(false); - const {admin, avatarSaturation, member, commentsEnabled, dispatchAction} = useContext(AppContext); - let comment = props.comment; - - useEffect(() => { - // This doesn't work, and should receive an update. - // When one Comment shows reply, while a different Form hides the reply form at the same time, the global - // 'isEditing' is unreliable. We should use a counter of total open forms instead of a boolean. - updateIsEditing?.(isInReplyMode || isInEditMode); - }, [updateIsEditing, isInReplyMode, isInEditMode]); - const toggleEditMode = () => { - setIsInEditMode(current => !current); - }; const closeEditMode = () => { setIsInEditMode(false); }; + const openEditMode = () => { + setIsInEditMode(true); + }; + + if (isInEditMode) { + return ( + + ); + } else { + return (); + } +} + +function Comment({comment, parent, openEditMode}) { + const isPublished = isCommentPublished(comment); + + if (isPublished) { + return (); + } + return (); +} + +function PublishedComment({comment, parent, openEditMode}) { + const [isInReplyMode, setIsInReplyMode] = useState(false); + const {dispatchAction} = useContext(AppContext); + const toggleReplyMode = async () => { if (!isInReplyMode) { // First load all the replies before opening the reply model @@ -52,100 +71,196 @@ const Comment = ({updateIsEditing = null, isEditing, ...props}) => { setIsInReplyMode(false); }; - const hasReplies = comment.replies && comment.replies.length > 0; - const isNotPublished = comment.status !== 'published'; - const html = {__html: comment.html}; + const hasReplies = isInReplyMode || (comment.replies && comment.replies.length > 0); + const avatar = (); + + return ( + + + + + + + + + ); +} + +function UnpublishedComment({comment, openEditMode}) { + const {admin} = useContext(AppContext); let notPublishedMessage; - if (isNotPublished) { - if (admin && comment.status === 'hidden') { - notPublishedMessage = 'This comment has been hidden.'; - } else { - notPublishedMessage = 'This comment has been removed.'; - } + if (admin && comment.status === 'hidden') { + notPublishedMessage = 'This comment has been hidden.'; + } else { + notPublishedMessage = 'This comment has been removed.'; } + // TODO: consider swapping this with a seperate avatar component + const blankAvatar = (); + const hasReplies = comment.replies && comment.replies.length > 0; + + return ( + +
+
+

{notPublishedMessage}

+
+ +
+
+
+ + + ); +} + +// Helper components + +function MemberBio({comment}) { + const {member} = useContext(AppContext); + const memberBio = member && comment.member && comment.member.uuid === member.uuid ? member.bio : comment?.member?.bio; + + if (!memberBio) { + return null; + } + + return ( + {memberBio}· + ); +} + +function EditedInfo({comment}) { + if (!comment.edited_at) { + return null; + } + return ( + + ·Edited + + ); +} + +function RepliesContainer({comment}) { + const hasReplies = comment.replies && comment.replies.length > 0; + + if (!hasReplies) { + return null; + } + + return ( +
+ +
+ ); +} + +function ReplyForm({comment, isInReplyMode, closeReplyMode}) { + if (!isInReplyMode) { + return null; + } + + return ( +
+
+
+ ); +} + +function EditForm({comment, parent, close}) { + return ( + + ); +} + +// +// -- Published comment components -- +// + +// TODO: move name detection to helper +function AuthorName({comment}) { + const name = !comment.member ? 'Deleted member' : (comment.member.name ? comment.member.name : 'Anonymous'); + return ( +

+ {name} +

+ ); +} + +function CommentHeader({comment}) { + return ( +
+
+ +
+ + + {formatRelativeTime(comment.created_at)} + + +
+
+
+ ); +} + +function CommentBody({html}) { + const dangerouslySetInnerHTML = {__html: html}; + return ( +
+

+

+ ); +} + +function CommentMenu({comment, toggleReplyMode, isInReplyMode, openEditMode, parent}) { + // If this comment is from the current member, always override member + // with the member from the context, so we update the bio in existing comments when we change it + const {member, commentsEnabled} = useContext(AppContext); + const paidOnly = commentsEnabled === 'paid'; const isPaidMember = member && !!member.paid; - const canReply = member && (isPaidMember || !paidOnly); + const canReply = member && (isPaidMember || !paidOnly) && !parent; - // If this comment is from the current member, always override member - // with the member from the context, so we update the bio in existing comments when we change it - const memberBio = member && comment.member && comment.member.uuid === member.uuid ? member.bio : comment?.member?.bio; + return ( +
+ {} + {(canReply && )} + {} +
+ ); +} - if (isInEditMode) { - return ( - - ); - } else { - return ( - -
-
-
- -
- {((!props.isReply && hasReplies) || isInReplyMode) &&
} -
-
-
- {isNotPublished ? -
-

{notPublishedMessage}

-
- -
-
: -
-

{!comment.member ? 'Deleted member' : (comment.member.name ? comment.member.name : 'Anonymous')}

-
- - {memberBio && {memberBio}·} - {formatRelativeTime(comment.created_at)} - - -
-
} -
+// +// -- Layout -- +// - {!isNotPublished && -
-

-

} - - {!isNotPublished && ( -
- {} - {(canReply && (isNotPublished || !props.parent) && )} - {} -
- )} - - {hasReplies && -
- -
- } - - {isInReplyMode && -
- -
- } -
-
- - ); +function RepliesLine({hasReplies}) { + if (!hasReplies) { + return null; } -}; -export default Comment; + return (
); +} + +function CommentLayout({children, avatar, hasReplies}) { + return ( +
+
+
+ {avatar} +
+ +
+
+ {children} +
+
+ ); +} + +// +// -- Default -- +// + +export default AnimatedComment; diff --git a/apps/comments-ui/src/components/CommentsBox.js b/apps/comments-ui/src/components/CommentsBox.js index d426b53fd1..31b7780c1f 100644 --- a/apps/comments-ui/src/components/CommentsBox.js +++ b/apps/comments-ui/src/components/CommentsBox.js @@ -1,4 +1,4 @@ -import React, {useContext, useEffect, useState} from 'react'; +import React, {useContext, useEffect} from 'react'; import AppContext from '../AppContext'; import CTABox from './CTABox'; import Form from './Form'; @@ -50,11 +50,8 @@ const CommentsBoxTitle = ({title, showCount, count}) => { }; const CommentsBoxContent = (props) => { - // @todo: This doesn't work and should get replaced with a counter of total open forms. If total open forms > 0, don't show the main form. - const [isEditing, setIsEditing] = useState(false); - - const {pagination, member, comments, commentCount, commentsEnabled, title, showCount} = useContext(AppContext); - const commentsElements = comments.slice().reverse().map(comment => ); + const {pagination, member, comments, commentCount, commentsEnabled, title, showCount, secundaryFormCount} = useContext(AppContext); + const commentsElements = comments.slice().reverse().map(comment => ); const paidOnly = commentsEnabled === 'paid'; const isPaidMember = member && !!member.paid; @@ -77,6 +74,8 @@ const CommentsBoxContent = (props) => { } }, []); + const hasOpenSecundaryForms = secundaryFormCount > 0; + return ( <> @@ -85,7 +84,8 @@ const CommentsBoxContent = (props) => { {commentsElements}
- { !isEditing + {secundaryFormCount} + {!hasOpenSecundaryForms ? (member ? (isPaidMember || !paidOnly ? : ) : ) : null } diff --git a/apps/comments-ui/src/components/Form.js b/apps/comments-ui/src/components/Form.js index edb079d346..59be73c722 100644 --- a/apps/comments-ui/src/components/Form.js +++ b/apps/comments-ui/src/components/Form.js @@ -12,7 +12,7 @@ import {GlobalEventBus} from '../utils/event-bus'; let formId = 0; const Form = (props) => { - const {member, postId, dispatchAction, avatarSaturation} = useContext(AppContext); + const {member, postId, dispatchAction} = useContext(AppContext); const [isFormOpen, setFormOpen] = useState(props.isReply || props.isEdit ? true : false); const formEl = useRef(null); const [progress, setProgress] = useState('default'); @@ -24,6 +24,20 @@ const Form = (props) => { const memberName = member?.name ?? comment?.member?.name; const memberBio = member?.bio ?? comment?.member?.bio; + // Keep track of the amount of open forms + useEffect(() => { + if (!props.isEdit && !props.isReply) { + // General form + return; + } + + dispatchAction('increaseSecundaryFormCount'); + + return () => { + dispatchAction('decreaseSecundaryFormCount'); + }; + }, [dispatchAction, props.isEdit, props.isReply]); + let buttonIcon = null; if (progress === 'sending') { buttonIcon = ; @@ -437,7 +451,7 @@ const Form = (props) => {
- +