From 1c7b466e64cdec1b91decd374142dc23c35bfed1 Mon Sep 17 00:00:00 2001
From: Dan Ditomaso <dan.ditomaso@gmail.com>
Date: Tue, 4 Feb 2025 14:46:46 -0500
Subject: [PATCH] fix: styling fixes from code review

---
 .../PageComponents/Messages/ChannelChat.tsx   |  15 +-
 .../PageComponents/Messages/Message.tsx       | 143 +++++++++++-------
 src/components/PageLayout.tsx                 |   2 +-
 3 files changed, 95 insertions(+), 65 deletions(-)

diff --git a/src/components/PageComponents/Messages/ChannelChat.tsx b/src/components/PageComponents/Messages/ChannelChat.tsx
index 46423237..47c9095c 100644
--- a/src/components/PageComponents/Messages/ChannelChat.tsx
+++ b/src/components/PageComponents/Messages/ChannelChat.tsx
@@ -39,7 +39,6 @@ export const ChannelChat = ({
           scrollContainer.scrollTop -
           scrollContainer.clientHeight <
         100;
-
       if (isNearBottom) {
         messagesEndRef.current?.scrollIntoView({ behavior: "smooth" });
       }
@@ -52,8 +51,8 @@ export const ChannelChat = ({
 
   if (!messages?.length) {
     return (
-      <div className="flex flex-col h-full">
-        <div className="flex-1 flex items-center justify-center overflow-y-auto">
+      <div className="flex flex-col h-full w-full">
+        <div className="flex-1 flex items-center justify-center">
           <EmptyState />
         </div>
         <div className="flex-shrink-0 p-4 w-full bg-gray-900">
@@ -64,9 +63,9 @@ export const ChannelChat = ({
   }
 
   return (
-    <div className="flex flex-col h-full">
-      <div className="flex-1 overflow-y-auto" ref={scrollContainerRef}>
-        <div className="w-full min-h-full flex flex-col justify-end">
+    <div className="flex flex-col h-full w-full">
+      <div className="flex-1 overflow-y-scroll w-full" ref={scrollContainerRef}>
+        <div className="w-full h-full flex flex-col justify-end">
           {messages.map((message, index) => (
             <Message
               key={message.id}
@@ -77,10 +76,10 @@ export const ChannelChat = ({
               sender={nodes.get(message.from)}
             />
           ))}
-          <div ref={messagesEndRef} />
+          <div ref={messagesEndRef} className="w-full" />
         </div>
       </div>
-      <div className="flex-shrink-0 p-4 w-full bg-gray-900">
+      <div className="flex-shrink-0 mt-2 p-4 w-full bg-gray-900">
         <MessageInput to={to} channel={channel} />
       </div>
     </div>
diff --git a/src/components/PageComponents/Messages/Message.tsx b/src/components/PageComponents/Messages/Message.tsx
index 3a96fd3b..66268219 100644
--- a/src/components/PageComponents/Messages/Message.tsx
+++ b/src/components/PageComponents/Messages/Message.tsx
@@ -4,28 +4,45 @@ import { Avatar } from "@components/UI/Avatar";
 import type { Protobuf } from "@meshtastic/js";
 import * as Tooltip from "@radix-ui/react-tooltip";
 import { AlertCircle, CheckCircle2, CircleEllipsis } from "lucide-react";
+import type { LucideIcon } from "lucide-react";
 
-export interface MessageProps {
+const MESSAGE_STATES = {
+  ACK: "ack",
+  WAITING: "waiting",
+  FAILED: "failed",
+} as const;
+
+type MessageState = MessageWithState["state"];
+
+interface MessageProps {
   lastMsgSameUser: boolean;
   message: MessageWithState;
   sender?: Protobuf.Mesh.NodeInfo;
 }
 
 interface StatusTooltipProps {
-  state: MessageWithState["state"];
+  state: MessageState;
   children: React.ReactNode;
 }
 
-const getStatusText = (state: MessageWithState["state"]): string => {
-  switch (state) {
-    case "ack":
-      return "Message delivered";
-    case "waiting":
-      return "Waiting for delivery";
-    default:
-      return "Delivery failed";
-  }
-};
+interface StatusIconProps {
+  state: MessageState;
+  className?: string;
+}
+
+const STATUS_TEXT_MAP: Record<MessageState, string> = {
+  [MESSAGE_STATES.ACK]: "Message delivered",
+  [MESSAGE_STATES.WAITING]: "Waiting for delivery",
+  [MESSAGE_STATES.FAILED]: "Delivery failed",
+} as const;
+
+const STATUS_ICON_MAP: Record<MessageState, LucideIcon> = {
+  [MESSAGE_STATES.ACK]: CheckCircle2,
+  [MESSAGE_STATES.WAITING]: CircleEllipsis,
+  [MESSAGE_STATES.FAILED]: AlertCircle,
+} as const;
+
+const getStatusText = (state: MessageState): string => STATUS_TEXT_MAP[state];
 
 const StatusTooltip = ({ state, children }: StatusTooltipProps) => (
   <Tooltip.Provider>
@@ -46,78 +63,92 @@ const StatusTooltip = ({ state, children }: StatusTooltipProps) => (
   </Tooltip.Provider>
 );
 
-const StatusIcon = ({
-  state,
-  className,
-}: { state: MessageWithState["state"]; className?: string }) => {
+const StatusIcon = ({ state, className, ...otherProps }: StatusIconProps) => {
+  const isFailed = state === MESSAGE_STATES.FAILED;
   const iconClass = cn(
     className,
     "text-gray-500 dark:text-gray-400 w-4 h-4 flex-shrink-0",
   );
-  const Icon = (() => {
-    switch (state) {
-      case "ack":
-        return CheckCircle2;
-      case "waiting":
-        return CircleEllipsis;
-      default:
-        return AlertCircle;
-    }
-  })();
+
+  const Icon = STATUS_ICON_MAP[state];
   return (
     <StatusTooltip state={state}>
-      <Icon className={iconClass} />
+      <Icon
+        className={iconClass}
+        {...otherProps}
+        color={isFailed ? "red" : "currentColor"}
+      />
     </StatusTooltip>
   );
 };
 
-export const Message = ({ lastMsgSameUser, message, sender }: MessageProps) => {
-  const messageTextClass = cn(
-    "border-l-2 pl-4 break-words min-w-0",
-    message.state === "ack"
-      ? "text-gray-900 dark:text-white"
-      : "text-gray-500 dark:text-gray-400",
-    lastMsgSameUser
-      ? "border-gray-600 dark:border-gray-700"
-      : "border-gray-200 dark:border-gray-600",
+const getMessageTextStyles = (state: MessageState) => {
+  const isAcknowledged = state === MESSAGE_STATES.ACK;
+  const isFailed = state === MESSAGE_STATES.FAILED;
+  const isWaiting = state === MESSAGE_STATES.WAITING;
+
+  return cn(
+    "pl-2 break-words overflow-hidden",
+    isAcknowledged
+      ? "text-black dark:text-white"
+      : "text-black dark:text-gray-400",
+    isFailed && "text-red-500 dark:text-red-500",
   );
+};
+
+const TimeDisplay = ({ date }: { date: Date }) => (
+  <div className="flex items-center gap-2 flex-shrink-0">
+    <span className="text-xs text-gray-500 dark:text-gray-400 font-mono">
+      {date.toLocaleDateString()}
+    </span>
+    <span className="text-xs text-gray-500 dark:text-gray-400 font-mono">
+      {date.toLocaleTimeString(undefined, {
+        hour: "2-digit",
+        minute: "2-digit",
+      })}
+    </span>
+  </div>
+);
+
+export const Message = ({ lastMsgSameUser, message, sender }: MessageProps) => {
+  const messageTextClass = getMessageTextStyles(message.state);
+  const isFailed = message.state === MESSAGE_STATES.ACK;
 
   const baseMessageWrapper = cn(
-    "ml-12 flex items-start gap-2 w-full max-w-full",
-    lastMsgSameUser ? "mt-1" : "mt-4",
+    "flex items-center gap-2 w-full max-w-full pl-11",
     !lastMsgSameUser && "flex-wrap flex-grow",
   );
 
   const containerClass = cn(
-    "px-4 relative",
-    lastMsgSameUser ? "mt-0" : "mt-2",
+    "w-full px-4 relative",
+    lastMsgSameUser ? "mt-1" : "mt-2",
     !lastMsgSameUser && "pt-2",
   );
 
   return (
     <div className={containerClass}>
       {!lastMsgSameUser && (
-        <div className="flex items-center gap-2 mb-2">
-          <Avatar text={sender?.user?.shortName ?? "UNK"} />
-          <span className="font-medium text-gray-900 dark:text-white">
-            {sender?.user?.longName ?? "UNK"}
-          </span>
-          <span className="text-xs text-gray-500 dark:text-gray-400 font-mono">
-            {message.rxTime.toLocaleDateString()}
-          </span>
-          <span className="text-xs text-gray-500 dark:text-gray-400 font-mono">
-            {message.rxTime.toLocaleTimeString(undefined, {
-              hour: "2-digit",
-              minute: "2-digit",
-            })}
-          </span>
+        <div className="flex flex-wrap items-center gap-x-2 gap-y-1">
+          <div className="flex items-center gap-2 min-w-0">
+            <Avatar
+              text={sender?.user?.shortName ?? "UNK"}
+              className="flex-shrink-0"
+            />
+            <span className="font-medium text-gray-900 dark:text-white truncate">
+              {sender?.user?.longName ?? "UNK"}
+            </span>
+          </div>
+          <TimeDisplay date={message.rxTime} />
         </div>
       )}
       <div className={baseMessageWrapper}>
-        <div className="flex-1 min-w-0">
+        <div className="flex-1 min-w-0 max-w-full">
           <div className={messageTextClass}>{message.data}</div>
         </div>
-        <StatusIcon state={message.state} className="ml-auto" />
+        <StatusIcon
+          state={message.state}
+          className="ml-auto mr-6 flex-shrink-0"
+        />
       </div>
     </div>
   );
diff --git a/src/components/PageLayout.tsx b/src/components/PageLayout.tsx
index 79495bdc..f25eecab 100644
--- a/src/components/PageLayout.tsx
+++ b/src/components/PageLayout.tsx
@@ -49,7 +49,7 @@ export const PageLayout = ({
         </div>
         <div
           className={cn(
-            "flex h-full w-full flex-col overflow-y-auto",
+            "flex h-full w-full flex-col",
             !noPadding && "pl-3 pr-3 ",
           )}
         >