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

Custom table elemets #141

Merged
merged 9 commits into from
Jan 16, 2024
Merged

Custom table elemets #141

merged 9 commits into from
Jan 16, 2024

Conversation

ciaranschutte
Copy link
Contributor

@ciaranschutte ciaranschutte commented Jan 16, 2024

Description of changes

  • use tsc instead of ttsc for local dev
  • allow completely custom elements for TH and TD (pass the same api props to custom els)
  • styling fixes

Type of Change

  • Bug
  • Styling
  • New Feature

Checklist before requesting review:

  • Design (select one):
    • Matches design:
      • component sizes, spacing, and styles
      • font size, weight, colour
      • spelling has been double checked
    • No design provided, screenshot of changes included in PR
    • No changes to UI
  • UI Kit (select one):
    • New Components with storybook stories created
    • Modified Existing components and updates stories
    • No new UIKit components or changes
  • Feature is minimally responsive
  • Manual testing of UI feature
  • Add copyrights to new files
  • Connected ticket to PR

@ciaranschutte ciaranschutte marked this pull request as ready for review January 16, 2024 09:24
canSort={canSort}
onClick={header.column.getToggleSortingHandler()}
>
{isCustomHeader ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this ever be true? Asking b/c of return on line 160

Copy link
Member

Choose a reason for hiding this comment

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

nice catch, looks like this will always be falsey.

Copy link
Member

Choose a reason for hiding this comment

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

not sure I follow this thread, but we were talking about that meta property in a different PR, coincidentally.
https://github.com/icgc-argo/rdpc-ui/pull/147/files#diff-a68ddbc20eef704019906015d66875a95925225e4642a3a4b630cb9b6399c13dR740

That line there is the one that would give a truthy value to isCustomHeader here (per 133/137).

What am I missing for what you guys are agreeing on?

Copy link
Member

Choose a reason for hiding this comment

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

aah! I see it now! yep!! copy pasta

package.json Outdated
@@ -20,7 +20,7 @@
"build::ts": "tsc -p ./tsconfig.json",
"build::babel": "babel ./src --extensions .tsx,.ts,.js --out-dir ./dist",
"clean": "rm -rf dist && mkdir dist",
"declarations": "ttsc --emitDeclarationOnly",
"declarations": "tsc --emitDeclarationOnly",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have link/docs related to what's changing here just for my education?

Copy link
Member

@justincorrigible justincorrigible Jan 16, 2024

Choose a reason for hiding this comment

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

We use the TTypescript to apply absolute path handling in the "build" script, which OG TS doesn't do.
Removing this dependency would mean we cannot use absolute paths in UIKit.

ttsc is just ttypescript's tsc, which applies the patches.

Copy link
Member

Choose a reason for hiding this comment

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

If we wanted to use an alternative approach to this, in Arranger I prepatch TS itself, rather than using TTS.
That may solve whatever issues @ciaranschutte is finding in his local env.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, and from their Github, it's deprecated. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

oh, that bit I hadn't realised. we'll have to update UIKit to account for that (different scope, unless this is a blocker here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

putting back in ttsc for build

@ciaranschutte ciaranschutte merged commit 3ca60cd into develop Jan 16, 2024
1 check passed
@ciaranschutte ciaranschutte deleted the custom_table branch January 16, 2024 20:06
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