Merge pull request #12 from oclif/mdonnalley/code-review

Mdonnalley/code review
oclif · Oct 3, 2024 · 94d1418 · 94d1418
2 parents 0b0069d + eae8041
commit 94d1418
Show file tree

Hide file tree

Showing 4 changed files with 41 additions and 172 deletions.
diff --git a/examples/orientation.ts b/examples/orientation.ts
diff --git a/examples/overflow.ts b/examples/overflow.ts
@@ -60,22 +60,6 @@ printTable({
   titleOptions: {bold: true},
 })
 
-// I would expect this for wrapping on align-center:
-//
-// Wrap (aligned center)
-// ┌───────┬─────────┬─────┬───────────────────────────────────────────────────────────────────────────┐
-// │  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.            │
-// └───────┴─────────┴─────┴───────────────────────────────────────────────────────────────────────────┘
-
 printTable({
   columns: ['id', 'name', 'age', 'description'],
   data,
@@ -98,24 +82,9 @@ printTable({
   overflow: 'wrap',
   title: 'Wrap (aligned left)',
   titleOptions: {bold: true},
+  verticalAlignment: 'center',
 })
 
-// Similar for align-right:
-//
-// Wrap (aligned right)
-// ┌───────┬─────────┬─────┬───────────────────────────────────────────────────────────────────────────┐
-// │    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. │
-// └───────┴─────────┴─────┴───────────────────────────────────────────────────────────────────────────┘
-
 printTable({
   columns: ['id', 'name', 'age', 'description'],
   data,
@@ -126,4 +95,5 @@ printTable({
   overflow: 'wrap',
   title: 'Wrap (aligned right)',
   titleOptions: {bold: true},
+  verticalAlignment: 'bottom',
 })
diff --git a/src/table.tsx b/src/table.tsx
@@ -128,7 +128,7 @@ function formatTextWithMargins({
   const valueWithNoZeroWidthChars = String(value).replaceAll('', ' ')
   const spaceForText = width - padding * 2
 
-  if (stripAnsi(valueWithNoZeroWidthChars).length < spaceForText) {
+  if (stripAnsi(valueWithNoZeroWidthChars).length <= spaceForText) {
     const spaces = width - stripAnsi(valueWithNoZeroWidthChars).length
     return {
       text: valueWithNoZeroWidthChars,
@@ -139,12 +139,42 @@ function formatTextWithMargins({
   if (overflow === 'wrap') {
     const wrappedText = wrapAnsi(valueWithNoZeroWidthChars, spaceForText, {hard: true, trim: true, wordWrap: true})
     const {marginLeft, marginRight} = calculateMargins(width - determineWidthOfWrappedText(stripAnsi(wrappedText)))
-    const text = wrappedText.replaceAll('\n', `${' '.repeat(marginRight)}\n${' '.repeat(marginLeft)}`)
+
+    const lines = wrappedText.split('\n').map((line, idx) => {
+      const {marginLeft: lineSpecificLeftMargin} = calculateMargins(width - stripAnsi(line).length)
+
+      if (horizontalAlignment === 'left') {
+        if (idx === 0) {
+          // if it's the first line, only add margin to the right side (The left margin will be applied later)
+          return `${line}${' '.repeat(marginRight)}`
+        }
+
+        // if left alignment, add the overall margin to the left side and right sides
+        return `${' '.repeat(marginLeft)}${line}${' '.repeat(marginRight)}`
+      }
+
+      if (horizontalAlignment === 'center') {
+        if (idx === 0) {
+          // if it's the first line, only add margin to the right side (The left margin will be applied later)
+          return `${line}${' '.repeat(marginRight)}`
+        }
+
+        // if center alignment, add line specific margin to the left side and the overall margin to the right side
+        return `${' '.repeat(lineSpecificLeftMargin)}${line}${' '.repeat(marginRight)}`
+      }
+
+      // right alignment
+      if (idx === 0) {
+        return `${' '.repeat(Math.max(0, lineSpecificLeftMargin - marginLeft))}${line}${' '.repeat(marginRight)}`
+      }
+
+      return `${' '.repeat(lineSpecificLeftMargin)}${line}${' '.repeat(marginRight)}`
+    })
 
     return {
       marginLeft,
       marginRight,
-      text,
+      text: lines.join('\n'),
     }
   }
 
@@ -163,7 +193,6 @@ export function Table<T extends Record<string, unknown>>(props: TableOptions<T>)
     horizontalAlignment = 'left',
     maxWidth,
     noStyle = false,
-    orientation = 'horizontal',
     overflow = 'truncate',
     padding = 1,
     sort,
@@ -237,48 +266,6 @@ export function Table<T extends Record<string, unknown>>(props: TableOptions<T>)
     skeleton: BORDER_SKELETONS[config.borderStyle].separator,
   })
 
-  if (orientation === 'vertical') {
-    return (
-      <Box flexDirection="column" width={determineWidthToUse(columns, config.maxWidth)} paddingBottom={1}>
-        {title && <Text {...titleOptions}>{title}</Text>}
-        {processedData.map((row, index) => {
-          // Calculate the hash of the row based on its value and position
-          const key = `row-${sha1(row)}-${index}`
-          const maxKeyLength = Math.max(...Object.values(headings).map((c) => c.length))
-          // Construct a row.
-          return (
-            <Box
-              key={key}
-              borderTop
-              borderBottom={false}
-              borderLeft={false}
-              borderRight={false}
-              flexDirection="column"
-              borderStyle={noStyle ? undefined : 'single'}
-              borderColor={borderColor}
-            >
-              {/* print all data in key:value pairs */}
-              {columns.map((column) => {
-                const value = (row[column.column] ?? '').toString()
-                const keyName = (headings[column.key] ?? column.key).toString()
-                const keyPadding = ' '.repeat(maxKeyLength - keyName.length + padding)
-                return (
-                  <Box key={`${key}-cell-${column.key}`} flexWrap="wrap">
-                    <Text {...config.headerOptions}>
-                      {keyName}
-                      {keyPadding}
-                    </Text>
-                    <Text wrap={overflow}>{value}</Text>
-                  </Box>
-                )
-              })}
-            </Box>
-          )
-        })}
-      </Box>
-    )
-  }
-
   return (
     <Box flexDirection="column" width={determineWidthToUse(columns, config.maxWidth)}>
       {title && <Text {...titleOptions}>{title}</Text>}
@@ -407,7 +394,6 @@ 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.
   process.stdout.write('\n')
 }
 
@@ -429,8 +415,8 @@ export function printTables<T extends Record<string, unknown>[]>(
 
   const processed = tables.map((table) => ({
     ...table,
-    // adjust maxWidth to account for margin
-    maxWidth: determineConfiguredWidth(table.maxWidth, columns),
+    // adjust maxWidth to account for margin and columnGap
+    maxWidth: determineConfiguredWidth(table.maxWidth, columns) - (options?.columnGap ?? 0) * tables.length,
   }))
 
   const instance = render(

diff --git a/src/types.ts b/src/types.ts
@@ -113,7 +113,7 @@ export type TableOptions<T extends Record<string, unknown>> = {
    *
    * If you provide a number or percentage that is larger than the terminal width, it will default to the terminal width.
    *
-   * If you provide a number or percentage that is too small to fit the table, it will default to the width of the table.
+   * If you provide a number or percentage that is too small to fit the table, it will default to the minimum width of the table.
    */
   maxWidth?: Percentage | number
   /**
@@ -125,15 +125,15 @@ export type TableOptions<T extends Record<string, unknown>> = {
    */
   headerOptions?: HeaderOptions
   /**
-   * Border style for the table. Defaults to 'all'. Only applies to horizontal orientation.
+   * Border style for the table. Defaults to 'all'.
    */
   borderStyle?: BorderStyle
   /**
    * Color of the table border. Defaults to 'white' in dark terminals and 'black' in light terminals.
    */
   borderColor?: SupportedColor
   /**
-   * Align data in columns. Defaults to 'left'. Only applies to horizontal orientation.
+   * Align data in columns. Defaults to 'left'.
    */
   horizontalAlignment?: HorizontalAlignment
   /**
@@ -170,26 +170,7 @@ export type TableOptions<T extends Record<string, unknown>> = {
    */
   sort?: Sort<T>
   /**
-   * The orientation of the table. Defaults to 'horizontal'.
-   *
-   * If 'vertical', individual records will be displayed vertically in key:value pairs.
-   *
-   * @example
-   * ```
-   * ─────────────
-   *  Name Alice
-   *  Id   36329
-   *  Age  20
-   * ─────────────
-   *  Name Bob
-   *  Id   49032
-   *  Age  21
-   * ─────────────
-   * ```
-   */
-  orientation?: 'horizontal' | 'vertical'
-  /**
-   * Vertical alignment of cell content. Defaults to 'top'. Only applies to horizontal orientation.
+   * Vertical alignment of cell content. Defaults to 'top'.
    */
   verticalAlignment?: VerticalAlignment
   /**