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

Feat(conversation)#WB-3834 draft list message preview #620

Merged
merged 9 commits into from
Mar 12, 2025

Conversation

damienromito
Copy link

@damienromito damienromito commented Mar 10, 2025

Description

-Afficher la liste des destinataires plutôt que l’expéditeur (+avatar correspondant)
-Afficher le label “brouillon” devant le destinataire de chaque message
-Possibilité d’avoir aucun destinataire renseigné

Fixes

(Enter here Jira or Redmine ticket(s) links)

Type of change

Please check options that are relevant.

  • Chore (PATCH)
  • Doc (PATCH)
  • Bug fix (PATCH)
  • New feature (MINOR)

Which packages changed?

Please check the name of the package you changed

  • admin
  • app-registry
  • archive
  • auth
  • cas
  • common
  • communication
  • conversation
  • directory
  • feeder
  • infra
  • portal
  • session
  • test
  • tests
  • timeline
  • workspace

Tests

-should display a "draft" label when in draft
-should display all recipients without "to" label when in draft
-should not display any recipients if there are none when in draft

Reminder

  • Security flaws

  • Performance impacts (think bulk !)

  • Unit tests were replayed

  • Unit tests were added and/or changed

  • I have updated the reminder for the version including my modifications

  • All done ! 😃

@damienromito damienromito self-assigned this Mar 10, 2025
@damienromito damienromito marked this pull request as ready for review March 10, 2025 16:28
@damienromito damienromito changed the base branch from master to develop-b2school March 10, 2025 16:28
@damienromito damienromito requested review from jcbe-ode, juniorode, evanthonyly and Romu-C and removed request for juniorode March 10, 2025 16:29
@damienromito damienromito changed the title Feat(conversation)#WB-3834 draft list Feat(conversation)#WB-3834 draft list message preview Mar 10, 2025
Copy link
Contributor

@jcbe-ode jcbe-ode left a comment

Choose a reason for hiding this comment

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

2 petits ajustements à appliquer

@@ -4,9 +4,13 @@ import { MessageMetadata } from '~/models';

export interface RecipientListPreviewProps {
message: MessageMetadata;
hasPrefix?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
hasPrefix?: boolean;
head?: string;

@@ -17,7 +21,7 @@ export function RecipientListPreview({ message }: RecipientListPreviewProps) {
};
return (
<MessageRecipientList
head={t('at')}
head={hasPrefix ? t('at') : null}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
head={hasPrefix ? t('at') : null}
head={head}

ou
{...otherProps}

export function RecipientListPreview({ message }: RecipientListPreviewProps) {
export function RecipientListPreview({
message,
hasPrefix,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
hasPrefix,
head,

ou }, ...otherProps

senderDisplayName
{folderId === 'draft' && <RecipientListPreview message={message} />}
{folderId === 'outbox' && (
<RecipientListPreview message={message} hasPrefix />
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<RecipientListPreview message={message} hasPrefix />
<RecipientListPreview message={message} head={t('at')} />

@@ -24,21 +24,27 @@ export function MessagePreview({ message }: MessagePreviewProps) {
<IconUndo className="gray-800" title="message-response" />
)}

{'outbox' === folderId ? (
{folderId && ['outbox', 'draft'].includes(folderId) ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

Dans l'ancienne messagerie, les noms des dossiers étaient en majuscule.
La comparaison ici ne tient pas compte de la casse, on risque donc d'avoir des effets de bord en ouvrant une URL à l'ancien format.
=> Il faut que le paramètre folderId soit "normalisé", et c'est justement ce que fait le hook useSelectedFolder, à utiliser à la place de useParams

Copy link
Author

Choose a reason for hiding this comment

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

@Romu-C ce hook sert donc à ça !

Copy link
Author

@damienromito damienromito Mar 11, 2025

Choose a reason for hiding this comment

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

donc on remplace useSelectedFolder partout ? histoire d'être consistant

Copy link
Author

@damienromito damienromito Mar 11, 2025

Choose a reason for hiding this comment

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

je vois qu'il y a aussi une constante SYSTEM_FOLDER_ID . Ça serait mieux de l'utiliser non ?

export const SYSTEM_FOLDER_ID = { INBOX: 'inbox', OUTBOX: 'outbox', DRAFT: 'draft', TRASH: 'trash', }

=> folderId === SYSTEM_FOLDER_ID.DRAFT ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Il vaudrait mieux, oui (x2). A discuter plus en détails si le moindre doute persiste.

@damienromito damienromito requested a review from jcbe-ode March 11, 2025 14:19
@@ -12,7 +12,17 @@ export function RecipientAvatar({ recipients }: RecipientAvatarProps) {
const { t } = useTranslation('conversation');
const { recipientCount, url } = useRecipientAvatar(recipients);

if (recipientCount > 1) {
if (recipientCount === 0) {

Choose a reason for hiding this comment

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

Tu peux refacto pour éviter la répétition et virer le else inutile :

if (recipientCount === 1 && url) {
  return <Avatar
      alt={t('recipient.avatar')}
      size="sm"
      src={url}
      variant="circle"
    />;
}

const avatars = {
  0: {
    bg: 'bg-yellow-200',
    label: t('recipient.avatar.empty'),
    icon: <IconQuestionMark className="w-16" />
  },
  1: {
    bg: 'bg-orange-200',
    label: t('recipient.avatar.group'),
    icon: <IconGroupAvatar className="w-16" />
  },
};

const avatar = avatars[Math.min(recipientCount, 1)];

return avatar ? (
  <div
    className={`${avatar.bg} avatar avatar-sm rounded-circle`}
    aria-label={avatar.label}
    role="img"
  >
    {avatar.icon}
  </div>
) : null;

@damienromito damienromito merged commit d443f7d into develop-b2school Mar 12, 2025
@damienromito damienromito deleted the feat/#WB-3834-draft-list branch March 12, 2025 10:42
damienromito added a commit that referenced this pull request Mar 12, 2025
* feat(conversation): #WB-3834 draft - show recipient rather than sender in message list

* feat(conversation): #WB-3834 draft - show draft label before recipient list

* feat(conversation): #WB-3834 draft - hide "at" label before recipient list + improve accessibility

* feat(conversation): #WB-3834 draft - possible to have any recipients

* feat(conversation): #WB-3834 draft - add empty recipient icon

* fix(conversation): #WB-3834 draft - review

* fix(conversation): #WB-3834 draft - review - add system folder id

* fix(conversation): #WB-3834 draft - review

* fix(conversation): #WB-3834 draft -  add SystemFolder type for useSelectedFolder hook
damienromito added a commit that referenced this pull request Mar 12, 2025
* feat(conversation): #WB-3834 draft - show recipient rather than sender in message list

* feat(conversation): #WB-3834 draft - show draft label before recipient list

* feat(conversation): #WB-3834 draft - hide "at" label before recipient list + improve accessibility

* feat(conversation): #WB-3834 draft - possible to have any recipients

* feat(conversation): #WB-3834 draft - add empty recipient icon

* fix(conversation): #WB-3834 draft - review

* fix(conversation): #WB-3834 draft - review - add system folder id

* fix(conversation): #WB-3834 draft - review

* fix(conversation): #WB-3834 draft -  add SystemFolder type for useSelectedFolder hook
damienromito added a commit that referenced this pull request Mar 12, 2025
* feat(conversation): #WB-3834 draft - show recipient rather than sender in message list

* feat(conversation): #WB-3834 draft - show draft label before recipient list

* feat(conversation): #WB-3834 draft - hide "at" label before recipient list + improve accessibility

* feat(conversation): #WB-3834 draft - possible to have any recipients

* feat(conversation): #WB-3834 draft - add empty recipient icon

* fix(conversation): #WB-3834 draft - review

* fix(conversation): #WB-3834 draft - review - add system folder id

* fix(conversation): #WB-3834 draft - review

* fix(conversation): #WB-3834 draft -  add SystemFolder type for useSelectedFolder hook
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.

4 participants