Skip to content

501: modify TextArea for user tags#506

Open
DarrellRoberts wants to merge 6 commits into
developfrom
darrell/feature/tag-users
Open

501: modify TextArea for user tags#506
DarrellRoberts wants to merge 6 commits into
developfrom
darrell/feature/tag-users

Conversation

@DarrellRoberts
Copy link
Copy Markdown
Collaborator

@DarrellRoberts DarrellRoberts commented May 8, 2026

Description

Changes TextArea so that it accepts user tags with @ character as well as display comments, so that tags look different from usual text

Related Issues

Closes #501

Changes

  • Bullet list of meaningful changes (optional)

Screenshots / Demos

image

Checklist

  • WITHIN THE SCOPE OF AN ISSUE; No unnecessary files included
  • Tests added/updated
  • Documentation updated
  • CI passes

@DarrellRoberts DarrellRoberts self-assigned this May 8, 2026
@need4deed need4deed self-requested a review May 18, 2026 13:06
@nadavosa
Copy link
Copy Markdown
Collaborator

Broken link href in CommentDisplay
Tags in displayed comments render as <Link href="/volunteers/:userId">:userId is a literal string, not a real param. Clicking any mention in a rendered comment navigates to a 404. Until BE user-resolution is wired up, render a styled <span> instead of a <Link>:

return <span key={...} className="tag">{value}</span>;

useCommentTag doesn't need to be a hook
It has no state, no effects, and calls no other hooks — it's a pure utility function. Naming it use… implies React rules apply (can only be called at the top level, etc.) which is misleading. Move it to a utils.tsx file and export it directly:

export function renderHighlightedText(value: string) { ... }

Variable shadowing
In the .map callback, the parameter is also named value, shadowing the outer value: string parameter:

const renderHighlightedText = (value: string) => {
  const parts = value.split(...);
  return parts.map((value, idx) => {  // ← shadows outer `value`

Rename the inner param to part.

user-selects: none CSS typo
The property is user-select. Browsers silently ignore the misspelled form.

Fixed height regresses UX
NewCommentSection now has height: 112px (was min-height: 112px + resize: vertical). Long comments scroll inside a fixed box instead of expanding. A minimum height with a maximum cap is friendlier:

min-height: 112px;
max-height: 300px;
overflow-y: auto;

No focus indicator on TextArea
Removing border-color from :focus and setting outline: none with nothing to replace it leaves keyboard users with no visible focus indicator — accessibility regression. Since the border is now on NewCommentSection, move the focus style there via :focus-within.

Copy link
Copy Markdown
Collaborator

@nadavosa nadavosa left a comment

Choose a reason for hiding this comment

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

The overlay scroll-sync technique is the right approach for this kind of rich-text highlight. A few things need to be resolved before merging:

Blocking:

  1. All three TODOs are unresolved and the links are broken. Every @mention currently navigates to the literal string /volunteers/:userId. Clicking a tag will land on a 404. This needs to be either wired up to a real user lookup, or the Link replaced with a plain styled span until the lookup is implemented.

  2. CSS typo: user-selects: none in TagOverlay should be user-select: none (no trailing s). As written the rule has no effect — the overlay will intercept pointer events even though pointer-events: none handles the main case.

Minor:

  1. Removing resize: vertical on the textarea and fixing height to 112px is a UX regression compared to the original min-height: 112px with vertical resize. Consider keeping resize: none but using min-height so longer comments are not cut off.

  2. aspect-ratio: 4/1 on AddCommentButton will break at narrow viewport widths — the button will shrink. A fixed min-width would be safer.

@DarrellRoberts
Copy link
Copy Markdown
Collaborator Author

hey @nadavosa , thanks for checking! Majority of these issues were fixed by PR #537 as the TODOs related to this PR as well as other issues will have crossed-over.

Copy link
Copy Markdown
Collaborator

@nadavosa nadavosa left a comment

Choose a reason for hiding this comment

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

Hey @DarrellRoberts — you mentioned the issues from my last review were fixed in PR #537. Looking at this PR's diff directly, a few things are still unresolved here:

Still open in this PR:

  1. Broken link href="/volunteers/:userId" in hooks/useCommentTag.tsx:userId is a literal string, not an interpolated value. Every @mention in a displayed comment navigates to a 404. PR #537's version of useCommentTag replaces this with a <span>, which is the right call until user-ID resolution is wired up. This PR needs the same fix or should be superseded by #537.

  2. user-selects: none CSS typo in styles.ts TagOverlay — the property is user-select (no trailing s). This PR's styles.ts still has the misspelled version. PR #537 has user-select: none (correct).

  3. Fixed height: 112px on NewCommentSection — raised in my last review; still unchanged here.

  4. No :focus-within on NewCommentSection — accessibility regression still present.


Suggestion on merge order: PR #537 supersedes useCommentTag and the styles — if #537 is ready first, this PR may need a rebase/update to avoid conflicts and to pick up the correct versions. Worth coordinating.

@DarrellRoberts
Copy link
Copy Markdown
Collaborator Author

thanks @nadavosa , tbh I would suggest closing the PR and just focus on PR #537 . This is the first task and PR #537 is built from it

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.

modify comments TextArea component so that user tag changes colour

3 participants