-
Notifications
You must be signed in to change notification settings - Fork 0
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
New repo code review #11
Conversation
@@ -63,6 +63,9 @@ const projects = [ | |||
}, | |||
] | |||
|
|||
// Occasionally, if you have two maxWidths that add up to 100% they will stack vertically instead of horizontally. | |||
// At first I thought this might be when the window has an odd number of pixels, but it seems to be more random than that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line: '', | ||
right: '', | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far things are looking good! I had two ideas for future features while reviewing:
|
src/table.tsx
Outdated
@@ -407,6 +407,7 @@ export function Skeleton(props: React.PropsWithChildren & {readonly height?: num | |||
export function printTable<T extends Record<string, unknown>>(options: TableOptions<T>): void { | |||
const instance = render(<Table {...options} />) | |||
instance.unmount() | |||
// It might be nice to have a "paddingBefore" and "paddingAfter" option for the number of newlines to enter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is necessary. People can print as many newlines as they want before or after the table. This new line only exists because, otherwise, the next output will appear on the same line as the last line of the table. For example
┌───────┬─────────┬─────┬───────────────────────────────────────────────────────────────────────────┐
│ Id │ Name │ Age │ Description │
├───────┼─────────┼─────┼───────────────────────────────────────────────────────────────────────────┤
│ 36329 │ Alice │ 20 │ Lorem ipsum dolor sit amet, consectetur adipiscing elit. Sed do eiusmod │
│ │ │ │ tempor incididunt ut labore et dolore magna aliqua. │
├───────┼─────────┼─────┼───────────────────────────────────────────────────────────────────────────┤
│ 49032 │ Bob │ 21 │ Lorem ipsum dolor sit amet, consectetur adipiscing elit. Sed do eiusmod │
│ │ │ │ tempor incididunt ut labore et dolore magna aliqua. │
├───────┼─────────┼─────┼───────────────────────────────────────────────────────────────────────────┤
│ 51786 │ Charlie │ 22 │ Lorem ipsum dolor sit amet, consectetur adipiscing elit. Sed do eiusmod │
│ │ │ │ tempor incididunt ut labore et dolore magna aliqua. │
└───────┴─────────┴─────┴───────────────────────────────────────────────────────────────────────────┘my next line
I actually had a streaming table working at one point but ripped it out because we didn't think there would be a valid use case inside of sf. I believe the consensus was that any table that had enough data to need streaming would be too big for the terminal. Like if a query in one of the data commands streamed in 1 million records, the user would be able to see those values populate as they came in |
I really liked this idea but I don't think it's worth implementing. You have two options: |
WIP
Review for @W-16891787@