Skip to content

Conversation

@darkexplosiveqwx
Copy link
Contributor

@darkexplosiveqwx darkexplosiveqwx commented Nov 21, 2025

My commits are currently not DCO signed, since I will squash to fewer commits later. I found out how to amend the DCO to multiple commits

What does this PR aim to accomplish?:

Update datatables.net-bs, currently 1.10.21 to 1.11.5, possibly to the latest 1.x (1.13.11)

How does this PR accomplish the above?:

All files are from https://cdn.datatables.net/v/bs/dt-VERSION/datatables{,.min}.{js,css}

I have added the source files and made an individual commit for each version so reviewers can quickly see what changed in the update. The source files will be force-pushed away later.
Changelogs are found at: https://cdn.datatables.net/VERSION/, like https://cdn.datatables.net/1.10.21/ , but not all changes apply to our Bootstrap 3 version

1.10.24 (e580e76) requires a CSS change for LCARS and the high-contrast themes (they are the only ones that style the default sorting indicator).
In addition to the soriting_asc/sorting_desc is now the sorting class applied. See DataTables/DataTablesSrc@a9ccfb4#r49603378
My fix in f33ccd4 makes it look the same as development/master.
What LCARS would look like without the fix:
Screenshot from 2025-11-21 15-26-25
With the fix / development:
Screenshot from 2025-11-21 15-30-45

Updating to 1.12.0 and later would require additional changes to the sorting indicators, since it changed the default sorting indicator.
Midnight on 1.11.5 and below (so also current development):
Screenshot from 2025-11-21 15-32-52
Midnight after 1.12.0:
Screenshot from 2025-11-21 15-34-04

Changelog for 1.12.0: https://cdn.datatables.net/1.12.0/
A diff can be found on another branch of mine: darkexplosiveqwx@ea72658

Should I update to 1.12.0 and later and just remove all the custom CSS for the sorting indicator?
Here is what it would look like for LCARS and High-contrast dark:
Screenshot from 2025-11-21 15-38-44
Screenshot from 2025-11-21 15-40-46

My other branch https://github.com/darkexplosiveqwx/pi-hole-web/tree/datatables-misc is updated to 1.13.11 and uses the new default sorting indicators


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code and I have tested my changes.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)
  6. I have checked that another pull request for this purpose does not exist.
  7. I have considered, and confirmed that this submission will be valuable to others.
  8. I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  9. I give this submission freely, and claim no ownership to its content.

  • I have read the above and my PR is ready for review. Check this box to confirm

@rdwebdesign
Copy link
Member

Should I update to 1.12.0 and later and just remove all the custom CSS for the sorting indicator?

Go ahead and let's see how it looks.
(Apparently my idea for LCARS indicators is almost exactly the same indicator used on the most recent versions.)

@rdwebdesign
Copy link
Member

and made an individual commit for each version so reviewers can quickly see what changed in the update.

I don't think this is necessary, unless you find a real issue that requires a lot of changes (or if you prefer to do this way).

@yubiuser
Copy link
Member

Thanks for picking this up. I tried once (#2852) but it caused so much trouble that I reverted the changes. But if you want to address all the necessary changes I'm all in for it.

@yubiuser
Copy link
Member

since it changed the default sorting indicator.

I prefer the look of the old sorting indicator, but this might be only because I'm used to it. If the only way to go forward it to use the new icon then I'll probably need to re-adapt.

Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
@darkexplosiveqwx
Copy link
Contributor Author

and made an individual commit for each version so reviewers can quickly see what changed in the update.

I don't think this is necessary, unless you find a real issue that requires a lot of changes (or if you prefer to do this way).

I found it helpful to look at the diff in addition to the changelog, so I decided to leave it in.
I have force-pushed the source files away and reduced the library update to a singe commit to simplify, if someone still wants to look at diffs, they are present on my other branch: https://github.com/darkexplosiveqwx/pi-hole-web/tree/datatables-misc

Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
@darkexplosiveqwx
Copy link
Contributor Author

I updated to 1.13.11 on this branch and changed it to use the default indicators for the high-contrast themes and LCARS

color: #eee;
}

.table-striped > tbody > tr:nth-of-type(2n + 1) {
Copy link
Member

@rdwebdesign rdwebdesign Nov 22, 2025

Choose a reason for hiding this comment

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

The new CSS changed the selector here, so we need to changed it to make sure the table rows have alternating background colors:

Suggested change
.table-striped > tbody > tr:nth-of-type(2n + 1) {
table.dataTable.table-striped > tbody > tr:nth-of-type(2n + 1) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about using tr.odd instead of tr:nth-of-type(2n + 1)?

Suggested change
.table-striped > tbody > tr:nth-of-type(2n + 1) {
table.dataTable.table-striped > tbody > tr.odd {

From the Domains page:

<tbody>
    <tr
        class="odd"
        data-id="0064006f006d00610069006e0031002e0063006f006d_deny_exact"
    ></tr>
    <tr
        class="even"
        data-id="0064006f006d00610069006e0032002e0063006f006d_deny_exact"
    ></tr>
    <tr
        class="odd"
        data-id="0064006f006d00610069006e0033002e0063006f006d_deny_exact"
    ></tr>
    <tr
        class="even"
        data-id="0064006f006d00610069006e0034002e0063006f006d_deny_exact"
    ></tr>
</tbody>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both table.dataTable.table-striped > tbody > tr.odd and table.dataTable.table-striped > tbody > tr:nth-of-type(2n + 1) seem to work for Domains, Groups, Clients, Lists and Query Log.
But neither seems to work for the "homepage":
Example using Midnight:
This is with table.dataTable.table-striped > tbody > tr.odd and table.dataTable.table-striped > tbody > tr:nth-of-type(2n + 1):
Screenshot from 2025-11-23 10-56-19
Screenshot from 2025-11-23 10-59-13

The old .table-striped > tbody > tr:nth-of-type(2n + 1) is working for the homepage, but not something like Domains:
Screenshot from 2025-11-23 10-58-02
Screenshot from 2025-11-23 10-58-24

The table on the homepage does not have even and odd classes.

We would need something like this for default-dark.css:

.table-striped > tbody > tr:nth-of-type(2n + 1) {
  background-color: #2d343a;
}
table.dataTable.table-striped > tbody > tr.odd {
  background-color: #2d343a;
}

The same is true for all other themes as well.
For LCARS:

Suggested change
.table-striped > tbody > tr:nth-of-type(2n + 1) {
table.dataTable.table-striped > tbody > tr.odd {
background: none;
background-color: rgba(80, 80, 80, 0.1);
}
.table-striped > tbody > tr:nth-of-type(2n + 1) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For High contrast light we dont seem to need it at all.
With

.table-striped > tbody > tr:nth-of-type(2n + 1) {
  background-color: #fcfcfc;
}
table.dataTable.table-striped > tbody > tr.odd {
  background-color: #fcfcfc;
}
Screenshot from 2025-11-23 11-10-11 Screenshot from 2025-11-23 11-10-36

Without:
Screenshot from 2025-11-23 11-12-10
Screenshot from 2025-11-23 11-12-34

We might want to keep it, but use a color with better contrast instead, I will commit the Without changes for now.

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm...

I only used the group management pages for testing my suggestions.

Your suggestions will probably fix all issues, but I will take a look an check (using the last commit) why some tables are working differently, to make sure nothing else is broken.

border: 1px solid #111 !important;
}

table.dataTable thead .sorting::before {
Copy link
Member

Choose a reason for hiding this comment

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

I agree most of the old CSS can be removed here, but I still think the default opacity is too low and the controls look too faded in this theme.

Can you please try these CSS rules?

table.dataTable thead > tr > th.sorting::before {
  bottom: calc(50% + 2px);
  opacity: 0.2;
}

table.dataTable thead > tr > th.sorting::after {
  opacity: 0.2;
}

table.dataTable thead > tr > th.sorting.sorting_asc::before {
  opacity: 1;
}

table.dataTable thead > tr > th.sorting.sorting_desc::after {
  opacity: 1;
}

background-color: rgba(0, 64, 64, 0.35);
}

table.dataTable thead .sorting::after,
Copy link
Member

Choose a reason for hiding this comment

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

We need to try co increase the contrast on these 2 themes

Can you try this for the dark theme?

table.dataTable thead > tr > th.sorting::before {
  opacity: 0.4;
}

table.dataTable thead > tr > th.sorting::after {
  opacity: 0.4;
}

table.dataTable thead > tr > th.sorting:hover::before,
table.dataTable thead > tr > th.sorting:hover::after {
  color: var(--primary-color);
}

table.dataTable thead > tr > th.sorting.sorting_asc::before,
table.dataTable thead > tr > th.sorting.sorting_desc::after {
  opacity: 1;
  color: #fff;
}

Copy link
Contributor Author

@darkexplosiveqwx darkexplosiveqwx Nov 23, 2025

Choose a reason for hiding this comment

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

[deleted](Commented on the wrong suggestion)

background-color: rgba(255, 204, 0, 0.333);
}

table.dataTable thead .sorting:after,
Copy link
Member

Choose a reason for hiding this comment

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

And this for the light theme?

table.dataTable thead > tr > th.sorting::before {
  opacity: 0.5;
}

table.dataTable thead > tr > th.sorting::after {
  opacity: 0.5;
}

table.dataTable thead > tr > th.sorting:hover::before,
table.dataTable thead > tr > th.sorting:hover::after {
  color: var(--primary-color);
}

table.dataTable thead > tr > th.sorting.sorting_asc::before,
table.dataTable thead > tr > th.sorting.sorting_desc::after {
  opacity: 1;
  color: #fff;
}

Co-authored-by: RD WebDesign <github@rdwebdesign.com.br>
Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
Copy link
Member

@yubiuser yubiuser left a comment

Choose a reason for hiding this comment

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

I just checked the visuals on all themes briefly and they look fine.
I would consider this PR ready for review?

@darkexplosiveqwx
Copy link
Contributor Author

Originally I considered updating datatables-buttons and datatables-select as well, but I experimented a bit and wouldn't include them here. I think this PR could be ready for review now.

@darkexplosiveqwx darkexplosiveqwx marked this pull request as ready for review November 23, 2025 11:53
@darkexplosiveqwx darkexplosiveqwx requested a review from a team as a code owner November 23, 2025 11:53
Copy link
Member

@yubiuser yubiuser left a comment

Choose a reason for hiding this comment

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

When selecting a row the background of the row turns blue, regardless of the theme. Before, the background changed accordingly to the theme and was never that 'intrusive'. Is this by design?

@yubiuser
Copy link
Member

Side question: is there any (technical) reason not to upgrade to the v2 version of data tables?

@rdwebdesign
Copy link
Member

rdwebdesign commented Nov 23, 2025

Is this by design?

I don't think so.
I need to check, but apparently some CSS classes were changed/added/removed from the tables dynamically created by the new version of the plugin and the previous CSS is failing to correctly style the rows.

EDIT:

There are CSS differences.

The old plugin version used a background color on the selected <tr> element:

table.dataTable tbody > tr.selected,
table.dataTable tbody > tr > .selected {
  background-color:#B0BED9
}

And we have our own rule on pihole.css to override this with our theme color.

Now, the new version applies a box-shadow (!) to all descendant elements of the selected row:

table.dataTable > tbody > tr.selected > * {
  box-shadow:inset 0 0 0 9999px rgb(0, 136, 204);
  box-shadow:inset 0 0 0 9999px rgb(var(--dt-row-selected));
  color:rgb(255, 255, 255);
  color:rgb(var(--dt-row-selected-text))
}

The background color is not changed, but the text color is changed.

Apparently there are many small CSS changes and we will need to adapt our themes.

@darkexplosiveqwx
Copy link
Contributor Author

Now, the new version applies a box-shadow (!) to all descendant elements of the selected row:

The changes are from the stylesheet modernization in datatables 1.12.0 .

From https://datatables.net/blog/2022/datatables-1.12#Row-colouring-improvements:

Row colouring improvements
If you've every tried to add a background colour to a table cell or row in DataTables you might have quickly run into problems with the colours of striped rows, order highlighted rows, FixedColumns and more. What initially appeared as a simple task could quickly become dense CSS depending on the features you were using. This was because DataTable's row tinting for striped rows, etc, was done with a background colour, leading to potential conflicts.

No longer! Our row tinting is now all done with an inset box-shadow (another benefit of modernising our CSS). With an alpha channel for the tint, you can now simply apply a background colour to cells and rows, and they will just work with all of DataTables features, including row selection.

Datatables now provides CSS variables for changing the color of cells:

:root {
  --dt-row-selected: 0, 136, 204;
  --dt-row-selected-text: 255, 255, 255;
  --dt-row-selected-link: 9, 10, 11;
  --dt-row-stripe: 0, 0, 0;
  --dt-row-hover: 0, 0, 0;
  --dt-column-ordering: 0, 0, 0;
  --dt-html-background: white;
}

I will try using the new CSS variables for changing the cell color whenever possible.

@rdwebdesign
Copy link
Member

rdwebdesign commented Nov 25, 2025

The issue is not just about the new datatables CSS, but how they interact with the AdminLTE v2.4 CSS and the changes made on top of that (pi-hole.css and specific theme files).

We will probably need to tweak the alpha channel for the box-shadow color, or simply remove the box-shadow (adding box-shadow: none to pi-hole.css) and create new CSS rules to change the row background-color on each theme.

EDIT:

I'm not able to test it right now, but reading the CSS, I think we can restore the previous behavior adding this to pi-hole.css:

table.dataTable tbody > tr.selected > *,
table.dataTable tbody > tr > .selected > * {
  box-shadow: none;
}

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.

3 participants