Skip to content

Conversation

@ethanluc7
Copy link
Contributor

@ethanluc7 ethanluc7 commented Jan 22, 2026

Related to issue: #375

Summary

Makes several updates to the schema table such as fixing text consistency, improving column behaviour via decreasing the width of the field column, and improving table header visual density.

Description of Changes

  • Added columnWidths prop to Table, TableHeader, and TableRow components
  • SchemaTable now passes explicit column widths: ['25%', '15%', '15%', '45%']
  • Headers apply widths via width CSS property
  • Cells apply widths via min-width CSS property
  • Reduced header vertical padding from 12px to 6px 12px
  • Reduced table header font size from 20px to 18px
  • "Unique value" message shortened: "A unique value that matches the following restrictions" → "Must have a unique
    value"
  • Added period to: "No restrictions provided for this field" → "No restrictions provided for this field."

Readiness Checklist

  • Self Review
    • I have performed a self review of code
    • I have run the application locally and manually tested the feature
    • I have checked all updates to correct typos and misspellings
  • Formatting
    • Code follows the project style guide
    • Autmated code formatters (ie. Prettier) have been run
  • Local Testing
    • Successfully built all packages locally
    • Successfully ran all test suites, all unit and integration tests pass
  • Updated Tests
    • Unit and integration tests have been added that describe the bug that was fixed or the features that were added
  • Documentation
    • All new environment variables added to .env.schema file and documented in the README
    • All changes to server HTTP endpoints have open-api documentation
    • All new functions exported from their module have TSDoc comment documentation

@ethanluc7 ethanluc7 requested a review from JamesTLopez January 22, 2026 14:10
@JamesTLopez
Copy link

As discussed offline, please remove the columnWidth implementation infavor of what we worked on.

Copy link

@JamesTLopez JamesTLopez left a comment

Choose a reason for hiding this comment

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

Small changes! But otherwise solid work 👍

Comment on lines +48 to +58
export const Mobile: Story = {
parameters: {
viewport: { defaultViewport: 'mobile1' },
},
};

export const MobileLarge: Story = {
parameters: {
viewport: { defaultViewport: 'mobile2' },
},
};

Choose a reason for hiding this comment

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

Minus tablet, I don't believe its beneficial to have views for Mobile and MobileLarge as mobile is not a point of focus.
Instead, we can have different story's for testing viewports in the ticket listed:

Test minimum column widths at common viewport sizes (1920px, 1440px, 1280px, 1024px)

If not thats fine, otherwise lets remove Mobile and MobileLarge stories

@@ -0,0 +1,64 @@
/*
*
* Copyright (c) 2025 The Ontario Institute for Cancer Research. All rights reserved

Choose a reason for hiding this comment

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

Its 2026 ;)

Comment on lines +40 to +41
max-width: 15vw;
min-width: 325px;

Choose a reason for hiding this comment

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

👍

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.

3 participants