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

Allow customizing row attributes #353

Merged
merged 1 commit into from
Nov 26, 2023

Conversation

SandroHc
Copy link
Contributor

Store the source row attributes, if available, to be used when rendering the table rows.

This introduces a breaking change in rowRender, as the signature of the row argument changed from cellType[] to { cells: cellType[], attributes: object }. I have updated Upgrading.md with the relevant entry for the next major version.

Tested all the demos and did not find regressions. Both DOM and JS init with data are supported; please see demo 25 for how that looks like.

The new tests are finicky when running under Chromium headless. Rows changed order every time I observed them, almost as if the table was being sorted and/or paged - schrodinger's tests. I could not reproduce it on my machine. The only way I managed to make the tests pass consistently was to make sure the tables being tested were small enough as to not have pagination and for the tests to not care about the order of the rows. ¯_(ツ)_/¯

<table>
    <thead>
        <tr>
            <th>Header 1</th>
            <th>Header 2</th>
        </tr>
    </thead>
    <tbody>
        <tr class="my-special-row">
            <td>Cell 1</td>
            <td>Cell 2</td>
        </tr>
    </tbody>
</table>
<script>
    new window.simpleDatatables.DataTable("table")
</script>

@johanneswilm
Copy link
Member

@SandroHc accessing and manipulating datatable.data.data will also be working differently with this change, right?

A breaking change is ok as this adds a major useful feature. But let's see if we can bundle it with other major changes so that we don't need to release a version 10 really soon after version 9.

@SandroHc
Copy link
Contributor Author

You're right, I forgot that those internals are exposed. I have updated the docs and upgrade guide to reflect that.

I have one more change planned, but it can be implemented in a backwards compatible way. It's to allow multiple classes via dt.options.classes. I will try to work on that next week.

@johanneswilm
Copy link
Member

I have one more change planned, but it can be implemented in a backwards compatible way. It's to allow multiple classes via dt.options.classes. I will try to work on that next week.

For some of them, it's perfectly fine to use a string containing multiple classes already. In other cases, it is used as a selector so that using something like "class-one class-one class-one" will break the datatable. In those cases, we probably need to choose which one is to be the selector. We could for example pick the first class ("class-one").

@@ -131,7 +146,7 @@ new DataTable("table", {
if (!tr.attributes) {
tr.attributes = {}
}
tr.attributes["data-name"] = rowValue[1].data
tr.attributes["data-name"] = rowValue.cells[1].data
Copy link
Member

Choose a reason for hiding this comment

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

@SandroHc Given that this is an example on how to upgrade to version 6.0.x, it seems like this should probably not be changed, right? But maybe this changed example could go under the 8->9 upgrade notes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've reverted it. Didn't mean to change it, but was in the middle of a fixing spree and didn't notice the file I was on.

## From 7.1.x to 8.0.x
### From 8.0.x to 9.0.x

* The type of [`datatable.data.data`](API#data) has changed from `object[]` to `{ cells: object[], attributes: object }[]`.
Copy link
Member

Choose a reason for hiding this comment

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

Could we add a simple example of what this change means? Just a simple: Instead of X, do Y. So that people who don't read every commit message and just quickly want to upgrade from one to another version quickly figure out what to do.

Copy link
Contributor Author

@SandroHc SandroHc Nov 24, 2023

Choose a reason for hiding this comment

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

Updated with examples that showcase the changes and the new functionality to customize the row attributes.

I messed up the commit rebase and GitHub is refusing to show the new changes. Please use this link to review them: https://github.com/fiduswriter/simple-datatables/compare/9a6f7f9600af050edbd3aaa945af819d8b40e848..7ccc14c19f765e2a0b954ddb4b743177f7a6813e

@SandroHc
Copy link
Contributor Author

For some of them, it's perfectly fine to use a string containing multiple classes already. In other cases, it is used as a selector so that using something like "class-one class-one class-one" will break the datatable. In those cases, we probably need to choose which one is to be the selector. We could for example pick the first class ("class-one").

I stumbled across the second case, options.classes.container is used as a selector. I'll try to squeeze a fix this weekend if I manage to get some time.

Store the source row attributes, if available, to be used when rendering the table rows.

This introduces a breaking change in `rowRender`, as the signature of the `row` argument changed from `cellType[]` to `{ cells: cellType[], attributes: object }`. I have updated `Upgrading.md` with the relevant entry for the next major version.

Tested all the demos and did not find regressions. Both DOM and programmatic insertion are supported; please see demo 25 for how that looks like.

```html
<table>
    <thead>
        <tr>
            <th>Header 1</th>
            <th>Header 2</th>
        </tr>
    </thead>
    <tbody>
        <tr class="my-special-row">
            <td>Cell 1</td>
            <td>Cell 2</td>
        </tr>
    </tbody>
</table>
<script>
    new window.simpleDatatables.DataTable("table")
</script>
```
@johanneswilm johanneswilm merged commit 9a032c2 into fiduswriter:main Nov 26, 2023
2 checks passed
@SandroHc SandroHc deleted the custom-row-attrs branch November 27, 2023 00:29
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