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

New repo code review #11

Merged
merged 17 commits into from
Oct 3, 2024
Merged

New repo code review #11

merged 17 commits into from
Oct 3, 2024

Conversation

iowillhoit
Copy link
Contributor

WIP

Review for @W-16891787@

@@ -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.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2024-10-03 at 10 19 33 AM Screenshot 2024-10-03 at 10 20 08 AM

line: '',
right: '',
},
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly don't care if you want to rename or delete this new style. Here is the difference:
Screenshot 2024-10-03 at 12 38 16 PM

@iowillhoit
Copy link
Contributor Author

So far things are looking good! I had two ideas for future features while reviewing:

  • iTerm2 supports annotations and is available in ansi-escapes. It would be neat to populate the full value of a line in an annotation if it was truncated.
  • For long running async processes, have a "live table" that would populate rows as they came in. This could also resize the table and adjust wraps/truncations if the window was resized. Similar to multi-stage-output

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.
Copy link
Contributor

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

@mdonnalley
Copy link
Contributor

For long running async processes, have a "live table" that would populate rows as they came in. This could also resize the table and adjust wraps/truncations if the window was resized. Similar to multi-stage-output

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

@mdonnalley
Copy link
Contributor

iTerm2 supports annotations and is available in ansi-escapes. It would be neat to populate the full value of a line in an annotation if it was truncated.

I really liked this idea but I don't think it's worth implementing.

You have two options: AddAnnotation and AddHiddenAnnotation. If you do the first, then every annotation box will show automatically, which will look messy. If you do the latter, then you can toggle the annotations with cmd + \ but that toggles all of them. Again, too messy

@iowillhoit iowillhoit merged commit dcbd133 into main Oct 3, 2024
11 checks passed
@iowillhoit iowillhoit deleted the ew/review branch October 3, 2024 21:23
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