dritter / extended-table Goto Github PK

In the code we have

{#if column.clickHandler}
  <td on:click|stopPropagation={() => column.clickHandler(d)} class="{getCellClasses(columnIndex, rowIndex, column, d)}" class:hidden={column.hidden}>
    ...
    <slot ...
    ...
  </td>
{:else}
  <td>
    ....
    <slot ...
    ...
{/if}

So because of the clickhandler we duplicate the config for 20 slots (massive code duplication). Not sure, if there is a way to avoid this.

Unfortunately I need this click handler..

WAI-ARIA FTW!

Just want to give back and share my edited version I'm using locally and point out some highlights and why's of the changes.

https://gist.github.com/fuzzthink/2b39d046ea1efca393623fc8c0af5687

Highlights of changes:

1. Use as single config object with key-vals of columns, rows, and pretty much all other configs. I've only added multisort because I want to override it to false.

• The main benefit is the source code is now rowsCfg.forEach((cfg) =>, {#each config.columns as cfg, iCol}, cfg.something, etc. This makes reading the code much easier as it takes no brain power to see it is a config, whereas column.something and row.something does -- it takes that extra brain power to recall it's config, and that's not a good thing. Yes, the code can be changed to this w/o changing the interface, but why not introduce this benefit to the caller too?
• The counterpart to it is the caller, it is much simpler <ExtTable data={rows} {config}>. And definition of config is clearly about config as well.

  const columns = [
    { propertyPath: 'acct', title: 'Acct', sortable: true },
   ...
  ]
  const config = {
    columns,
    rows: [...],
    ...rest,
  }

2. Take out the classNames code into its own file. I love how short your code is, but it short as it is, it can be better extracting non-related code into its own file. tableClassNames.js is just that.
3. Take advantage of js tooling and use ?. to simplify the logic. The current PR can be much easier to read with ?.. Take a look at tableClassNames.js, the logic is just 4 flat conditionals, no need for nesting ifs.
4. Repeating the same code just because one has a on:click|stopPropagation and one doesn't is code smell to me. 90% of errors happens because we forget to apply the same fix needed to all the different places. I'm probably missing the need for the stopPropagation, but until I run into that problem, I'm not going to let that mess up my code. And if I do run into it, I believe there should be better solutions.
5. Don't build something YAGNI. Supporting 20 columns of slots added 80 lines (40 lines if you take away the repeated code above). If you do need 20 cols of slots, don't make any changes (until svelte supports dynamic slot names). But don't ruin your code for something maybe one user out of 100/1000s may use it. For me, I think supporting just 1-3 cols in the start and end is enough.
6. Minor stuff, mostly dev preferences, but I like keeping var names short as long as it is more readable to me, using const over let to show intention, using if (...) instead of (...) && -- much easier to read imho, see tableClassNames.js, and using ternaries more (getOddEvenClass).

With these changes, the main file is just 200 lines and 260 if classNames logic is not extracted. More importantly is readability, which imho, has been enhanced.

Please don't take this the wrong way, I've already mentioned how much I like your library. Just want to share my thoughts so you can take some of these suggestions if you think they are sound.

It would be nice to test the library within github actions with cypress. There is even an official action!

It's about time.. ExtendedTable has reached a maturity level that can be released into the wild.

@dritter Thank you so much for such a great table library. I've looked at all table libraries for Svelte, and this is the best one. I have no idea why you aren't getting the traffic like others.

One feature I see lacking is there isn't a way to add css classes per row or cell other than using slots. It is extremely helpful if that is possible. One eg. of how I see they can be specified is introducing an optional rows prop that is declared similar to columns and className spec on columns props.

const data = [{
  value: 'a',
  count: 1,
  css: 'danger', // className on tr (row) specified
}, {
  value: 'b',
  count: 5,
  css: { count: 'success' }, // className on td (cell) specified
}, ...rest]

// new rows props spec
const rows = [{
    propertyPath: 'css',
    className: 'red', // optional value string or function for className on tr. If omitted, the value obtained via propertyPath is taken
}, ...otherRowsSpec]

const columns = [{ 
    title: 'Count', 
    propertyPath: 'count',
    className: { // new cell props spec
        propertyPath: 'css',
        value: (val) => val==='success' ? 'green' : "", // optional value string or function for className on td.
        // Just like for rows, if omitted, the value obtained via propertyPath is taken.
    }
}, ...otherColsSpec]

I would be thrilled if this is possible and believe others may be as well. Let me know what you think.

How fast are we? How fast are others?

The default styling is pretty ugly.. 😞

Mostly about defaults for sorting, but leaving title to just "defaults" if you like to talk more than sorting.

I'd like to hear your thoughts and your use case on these defaults.

I've changed my local src to set config to { ...defaultConfig, ...config } to override these defaults:

• sortable - for me, I don't find any reason to disable sorting on a col, so instead of adding sortable=true on every col, I like to just only define sortable=false as needed.
  FYI, the change for mouse-pointer logic is:

  if ((cfg.hasOwnProperty('sortable') && !cfg.sortable)) {
    // don't add mouse-pointer
  } else classes.push('mouse-pointer')

and to add defaults to columns:

  if (!sortingFunction) {
    sortingFunction = (cfg, columns, data, multisort, cacheClearCallback) => {
      cfg.sortable = cfg.hasOwnProperty('sortable') ? cfg.sortable : config.sortable
      return sortByColumn(cfg, columns, data, multisort, cacheClearCallback)
    }
  }
// Need more work if custom `sortingFunction` is passed.

• multisort - for me, I default it to off because:

1. the current UX does not help the user understand the multi-sorting
2. The desire to allow multi-sort even if UX is good is usually no