Make CSS Classes more flexible

[dritter]

Hey @fuzzthink ,

here I put a first draft of your original idea together. Hopefully this is not overengineered. It works (hopefully) pretty much as you suggested, with a few changes. The differences should be explained best in the docs or theming example code. WDYT? Anything missing?

resolves #7

Greetings,
dritter

[mationai]

Wow, you're amazing! I can't thank you enough. I will take a look later today and provide suggestions or changes if needed.

[mationai]

Hmm, I think my original thoughts on using "rows" for config as counterpart to existing "columns" config was a bad idea for several reasons.

1. Many devs like to name their data presented to be rendered "rows", so it will cause confusion to new devs looking at the code the first time and also to original coders when they go back to the code after a while.
2. More importantly, since "columns" config is order-mapped to the cols, using "rows" as config caused you to misunderstood how it is used.

The idea is that it is an array of css configurations, not an array of each individual rows. It does not make much real world sense to produce a styling map for thousands of rows, especially if the custom styling is sparse.

Let's rename the "rows" prop to "classNames" as it seems like a better name for its purpose.

My (and probably many users') use case is there is some fields of data that needs to be highlighted to bring to the attention of the reader. If a medium attention is needed, then just the cell needs to be highlighted. If a high attention is needed, then both the cell and the row
need to be highlighted.

In my example given in #7 , cc here:

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',
}, ...otherRowsSpec]

The data has already augmented with css: 'danger' on the row to indicate the row should get a 'danger' css class, and css: { count: 'success' } on the 2nd row to indicate only the count -- the cell should be classed.

In this case, the row styling config should be { propertyPath: 'css' }, which will look at all data rows for 'css' path and take that value and set the css class for the row. This setting will take care of cell targeting since the cell targeting is done in the data as css: { count: 'success' }. (I had className: 'red', as well in the config that was suppose allow override, but please just forget that, a bad idea, please ignore).

This is only an example where we allow the css class to be specified in the data. I agree with you as you mentioned that setting the css class in the data may not be ideal or best practice.

To enable css class setting by evaluating data value, this usage seems more thought out. Let's take a look at a slightly modified version.

const data = [{
  value: 'a',
  count: 1, // we want 'danger' on row
}, {
  value: 'b',
  count: 5, // we want 'success' on cell
}, ...rest]

// our new classNames config:
const classNames = [{
   propertyPath: 'css', // set className by data, not useful in this example
}, {
  rows: (rowData) => { // single config for row styling
    if (rowData.count <= 1 || rowData.value === 'Defect') return 'danger'
    // can do else if on other columns and return a className too
    return ''
  }
}, {
  cells: [{ // multiple configs for cell styling
    propertyPath: 'count',
    value: (rowData) => {
      if (rowData.count > 4) return 'success' // only highlight the cell if good count
      return ''
  }, {
    propertyPath: 'value',
    value: (rowData) => {
      if (rowData.value === 'Defect') return 'danger' // style the cell in addition to row
      return ''
  }]
}, {
  columns: [{ // multiple configs
    propertyPath: 'value',
    value: (data) => {
      for (let rowData of data) {
        if (rowData.value === 'Defect' ) // we want to highlight the col to indicate there's a row
          return 'red-border' // with Defect since the row maybe below the fold and missed.
      }
      return ''
    }
  }, {
    propertyPath: 'count',
    value: (data) => {
      for (let rowData of data) {
        if (rowData.count <= 1) 
          return 'red-border'
      }
      return ''
    }
  }]
}]

This way, all cell, row, and column targeted style can be set in a single prop.

Feel free to support only one of these configs (set in data, or set via config functions) if you feel it's not worth the extra effort.

I believe most will opt for the function config way. But if users are transforming their data anyways, they may opt to set it in the data for simpler render-side code.

Much appreciate your PR and sorry for the miscommunications.

[dritter]

Cool. Thanks for the input!

Much appreciate your PR and sorry for the miscommunications.

No worries. I like collaborating better on a feature than doing it all by myself and eventually producing a less optimal result. So, thank you very much for your feedback!

I like the idea of combining the className config in one object. I'll update the PR in the next days.

One thing I noticed in your example:

Having code like this:

{ // multiple configs for cell styling
    propertyPath: 'count',
    value: (rowData) => {
      if (rowData.count > 4) return 'success' // only highlight the cell if good count
      return ''
  },

will lead to having the count resolved written as class (in your example a Number) AND the value callback will write a success class. But you do not need the propertyPath to use value.

Tl;dr

This is what I'll do:

• combine the className config into one prop (former rows)
• rename value to callback (seems to be a better name)
• update docs and example

[dritter]

One quick thought:
Combining cell, row and header means that we have to match the right header (or cell) css config to the actual header/cell. This means that if we want to add a css class to the 3rd header column, that we need to add two empty ones first (matched by index).

const cssClasses = {
  header: [
    'headline__username',       // 1st column: Static class
    null,                       // 2nd column: No additional class, hence null
    {                           // 3rd column: Rules for adding a css class
        value: (columnDefinition) => columnDefinition.propertyPath + '-TEST',
    }
};

[mationai]

I like the idea of combining the className config in one object. I'll update the PR in the next days.

👍

will lead to having the count resolved written as class (in your example a Number) AND the value callback will write a success class. But you do not need the propertyPath to use value.

Without propertyPath, how can it know which cell user wants to add className to? Cell and header needs it to target the cell/column.
"will lead to having the count resolved written as class" -- this is only if 'propertyPath' is defined as one of the root item keys. I'm all for removing that feature (to support a "css" field in data), as it is not really needed. All it does is make the table rendering code cleaner but the logic is still needed to transform the data. It may be useful for shops that lack a decent js/FE dev, that's all I can think of.

• rename value to callback (seems to be a better name)

I actually prefer value more. You don't find much library what names an object key param as callback. We are returning the value for the className here, so I think it's appropriate. It matches the existing spec as well. Maybe className if you really dislike value?

• update docs and example

Feel free to use or derive from this example.

Combining cell, row and header means that we have to match the right header (or cell) css config to the actual header/cell. This means that if we want to add a css class to the 3rd header column, that we need to add two empty ones first (matched by index).

My thought and example is for headers to work the same way as rows. Instead of looping and requiring the user to specify many null just to specify one targeted header, the propertyPath targets the column.

[dritter]

Without propertyPath, how can it know which cell user wants to add className to? Cell and header needs it to target the cell/column.

This is not how it works. The propertyPath is not to connect config and data. Instead it is resolved on the current data item (the row). Maybe I should explain more how that process works:

At first we build up the rows by iterating over the data array. For every row we loop over the column definition (columns prop) and select the right value out of the current data item. This selection is either done via a configured slot for this column, or a value prop on the data item, or a propertyPath that is resolved on the data item. The value and propertyPath are configured on the column.

Similar to this process I modeled the selection of the correct css class. At the current state of this PR, we check - while iterating over the columns - if there is a className property configured on the column definition. So at this point we know the current column config and don't have to match that against a css config object.

In short:

• propertyPath is to select a value directly from the current data item
• value is a callback function that gets the current data item as parameter (next to column and row index)
• className as string writes the static string

My thought and example is for headers to work the same way as rows. Instead of looping and requiring the user to specify many null just to specify one targeted header, the propertyPath targets the column.

The difference between how cssClasses for rows and cells are applied is that for rows it works rolling. That it is not rolling for cells/headers is the culprit, why we need to add null configs to target a later column.
I sense that applying the css config in a rolling way makes most sense for the rows, but less for cells. I think this is so, because usually you don't want to target the top 20 rows, but more highlight single rows, or highlight some columns as they contain important information (more than others maybe).
As a dev dealing with config applied in a rolling manor is a bit more complex than adding a config to a column config, because you have to think more about how to target the column you want without accidentally targeting other columns.

So maybe the current state of the PR is not that bad.. But I agree that the prop name is misleading. How about something more expressive like rowCssClasses? Or rollingRowCssClasses?

I'm all for removing that feature (to support a "css" field in data), as it is not really needed.

In this PR we do not search for a fixed key in the data items. This connection is done if a propertyPath is set on the column definition that points to a arbitrary key in the data item.

rename value to callback (seems to be a better name)

I actually prefer value more.

Okay. Let's keep value then.

[mationai]

Thank you for the update @dritter

The cell and column targeting seems good.

However, I don't understand why the need for "rolling" row css definitions. Forcing it seems like it makes the feature unusable (at least to me). I don't see much real world usage of "rolling" row css definitions. It also makes the example and usage confusing.

I modified a local copy of the PR and in place of const row = rows[index % rows.length] || {};,
I enclose the block between that and return with for (let row of rows) { ... }. After changing the 3rd row spec in example to a more realistic one,

    {
      className: {
        value: (data, row, rowIndex) => data.title === "mr" ? "success" : ""
      }
    }

it did highlighted all rows with "mr" title as expected (ignoring the css bug of alternating background gray override the green).

Let me know your thoughts.

[mationai]

To clearify, const row = rows[index % rows.length] || {}; should be removed and replaced with a loop. Each row spec specifies a className a row gets, as long as the condition holds true.

[dritter]

Thanks for your feedback!

However, I don't understand why the need for "rolling" row css definitions. Forcing it seems like it makes the feature unusable (at least to me). I don't see much real world usage of "rolling" row css definitions. It also makes the example and usage confusing.

Flexibility. :) At least that was my initial thought. And a bit of consistency with the column definition. Here we have one definition for one column.. For rows that is not feasible, hence the idea of rolling the definitions. I thought that everything that is possible by iterating over the rows definition as you suggested should be possible with a rolling row definition:

Iterating version

let rowCssClasses = [
        {className: {value: (data, row, rowIndex) => data.title === "mr" ? "success" : ""}},
        {className: {value: (data, row, rowIndex) => data.username.startsWith('d') ? "my-other-class" : ""}},
    ];

should be equal to this rolling config:

let rowCssClasses = [
        {className: {value: (data, row, rowIndex) => {
            let classes = [];
            data.title === "mr" && classes.push("success");
            data.username.startsWith('b') && classes.push('my-other-class');
            return classes.join(" ");
        }}}
];

BUT, it turns out, that you are actually right. The rolling config above leads to rows with success_my-other-class as class. This is because of the values returned are sluggified, and all non-alphanumerical characters (like whitespaces) being replaced by underscores. This could be solved, but would make handling this case even more complicated. Therefore I'll change it as you suggested.

To clearify, const row = rows[index % rows.length] || {}; should be removed and replaced with a loop. Each row spec specifies a className a row gets, as long as the condition holds true.

Sounds good. Thanks for the suggestion!

css bug of alternating background gray override the green

Thanks for bringing this up. Indeed. This is due to svelte scoping the css of the component and raising specificity by adding a data attribute. Unfortunately there seems no good way of doing the opposite: adding default styling. So we are in the middle of specificity wars here. One way to win is to add more specificity on the outer side (wrapping the div in App.svelte with another div). Ugly, but works.

[mationai]

See comments

[mationai]

Replacing spaces with '_' prevents multiple classes to be returned by the same function. If we want to support that, the 2 solutions can be:

1. Don't replace spaces with '_'
2. Support arrays as return value and concat it if type is array.

[dritter]

Both ways would indeed be possible. I'll try to outline pros and cons here:

Not replacing spaces (supporting string-arrays)

On the pro side this feels pretty normal and is in a way unexpected, that we transform the result of the callback.

Support real arrays

This also feels pretty normal, as we are here in a JavaScript function, so why shouldn't we return an array?

The downside for both is the additional complexity (in code and for the user) and that we (if we would support both ways) allow multiple return types.

Future perspective

I tend to say that we wait for someone who needs this feature. Or do you need it today?
Looking into the future, if we defer this feature for now, it might count as a small bc break if the implement this later on (and omit replacing spaces with underscores). Introducing arrays seems not to infringe backwards compatibility.

Do you have a preference?

[dritter]

Another way would be to remove sluggification for values returned from the callback.. IMHO, sluggifying (or is it sluggification?) makes most sense with the propertyPath (which might be out of control of the dev). Removing it might fit the principle of least surprise best I guess..

[mationai]

Good thoughts.

I think sluggifying only propertyPath makes sense since that is non-dev control like you said and it makes sense to sluggify it as there is very little to zero chance devs wants spaces as class separators. value function on the other hand, devs would normally expect spaces to be honored, so sluggifying it would certainly surprise them.

I don't currently have the need for spaced string classes, but I can easily imagine it will come in handy later or to devs that uses css libs that requires it, like <topic> and <topic>-<specific> in order for the css to work at all.

So I would go with only sluggifying propertyPath and not value functions - easy to implement, and works as most would expect it to.

[dritter]

Thanks for the feedback. I removed sluggifying the value and added some documentation.

[mationai]

Looks good! 🥇

[dritter]

FYI: I have not merged yet, because I consider renaming the rowClasses prop again, because I could use that in the future for other features (like supporting multiple tbody elements, etc.).

Do you have concerns renaming this prop to rows again and making it an object like so:

<script>
let rowDefinition = {
        classNames: [
            {className: {propertyPath: 'location.postcode'}},
            {className: {value: (data, row, rowIndex) => (rowIndex + 1) % 2 === 0 ? data.last_name : ""}},
            {className: {value: (data, row, rowIndex) => data.title === "mr" ? "male my-other-class" : ""}},
        ],
};
</script>

<ExtendedTable columns={columnDefinition} data={data} rows={rowDefinition}></ExtendedTable>

[mationai]

I'm good with that. But why the need for extra depth of className? I don't see why there should be other keys if parent is already keyed as classNames.

So I think it is much nicer if it's shorter as

let rowDefinition = {
    classNames: [
        { propertyPath: 'location.postcode' },
        { value: (data, row, rowIndex) => (rowIndex + 1) % 2 === 0 ? data.last_name : "" },
        { value: (data, row, rowIndex) => data.title === "mr" ? "male my-other-class" : "" },
    ],
};

[dritter]

The additional level makes it easier in the future to add new features (like grouping rows in several tbodies; there it would be necessary to add a callback when to group). Speaking in SemVer, that would make it a minor version update, instead of a major one.

[mationai]

Can you please provide an example of how that looks like?

[mationai]

Forgot to point out something I noticed.

[mationai]

column is the current column spec defined, how is it useful in the function? Even if it is useful, it's accessible here as the user is defining it. Remove? Remove row in row config function too?

Edit: This comment is meant for the src, not this example.

[dritter]

Good catch! That parameter is indeed not very useful. I removed it.

[dritter]

Can you please provide an example of how that looks like?

Sure.

Speaking config-wise:

let rowDefinition = {
    classNames: [
        {value: (data, rowIndex) => data.title === "mr" ? "male my-other-class" : ""},
    ],
    group: (data, rowIndex) => {
        return rowIndex % 3 === 0;
    }
};

This config should group every three rows in a new tbody.

The generated HTML would look like in this example.

[mationai]

Thank you for the example. Didn't know about it. Not sure of its usefulness deviating from the default table structure. Like, for short grouped rows, it's more of a rendering thing, and for longer ones, does it make more sense to break into diff tables? I'll have to see some real world usages to form an opinion.