Skip to content

System Options nits#130

Merged
ChinemeremChigbo merged 6 commits intomainfrom
colin/system-options-nits
Apr 17, 2025
Merged

System Options nits#130
ChinemeremChigbo merged 6 commits intomainfrom
colin/system-options-nits

Conversation

@ColinToft
Copy link
Contributor

@ColinToft ColinToft commented Apr 15, 2025

Notion Ticket

Ticket Name

Summary & Review Focus

Fixes nits:

  • System options should not have individual text boxes for check / cancel, it should all be one solid block
  • Clicking edit while in system options scrolls you back to the top
  • The triple dot position should be essentially fixed and not be pushed to the right if there are too many letters (the letters should fade out instead)
  • Everyone on the platform should get enrolled when you make a new email sub
  • Display field in the system options fades out characters that are too long when clicking out

Testing Instructions

Checklist

  • PR title is descriptive and in imperative tense
  • Commit messages are descriptive, atomic, and follow best practices
  • Linter(s) have been run
  • Requested reviews from the PL and relevant team members

… this was in a portal but hopefully removing it doesn't break anything
@vercel
Copy link

vercel bot commented Apr 15, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
sistema ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 17, 2025 1:01am

@ColinToft ColinToft self-assigned this Apr 15, 2025
@ColinToft ColinToft marked this pull request as ready for review April 15, 2025 20:34
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

src/components/dashboard/system_options/EntityTable.tsx:609

  • The tooltip disable condition uses a threshold of 3 characters, which may not align with the requirement to display the first 12 characters; consider updating the condition to check if item.abbreviation.length is less than or equal to 12.
<Tooltip label={item.abbreviation} placement="top" openDelay={300} isDisabled={item.abbreviation.length <= 3} hasArrow>

app/api/system/batch-update/route.ts:26

  • Consider wrapping the subject creation and the subsequent mailing list insertions in a transaction to ensure atomicity, similar to the approach used in app/api/subjects/route.ts.
await tx.subject.create({

Copy link
Member

@ChinemeremChigbo ChinemeremChigbo left a comment

Choose a reason for hiding this comment

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

LGTM

@ChinemeremChigbo ChinemeremChigbo merged commit 6cda6cb into main Apr 17, 2025
3 checks passed
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.

2 participants