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

fix: exclude status checkboxes from being treated as iCheck elements #1910

Merged
merged 2 commits into from
Nov 9, 2021

Conversation

lionralfs
Copy link
Contributor

@lionralfs lionralfs commented Oct 2, 2021

By submitting this pull request, I confirm the following:

  • I have read and understood the contributors guide, as well as this entire template.
  • I have made only one major change in my proposed changes.
  • I have commented my proposed changes within the code.
  • I have tested my proposed changes.
  • I am willing to help maintain this change if there are issues with it later.
  • I give this submission freely and claim no ownership.
  • It is compatible with the EUPL 1.2 license
  • I have squashed any insignificant commits. (git rebase)
  • I have Signed Off all commits. (git commit --signoff)

What does this PR aim to accomplish?:

Fixes #1849.
In Safari, the toggle buttons under Group Management > Groups are broken.
This is due to the following phenomenon: The data table is rendered before these lines in footer.js:
https://github.com/pi-hole/AdminLTE/blob/50f43bde73d25211ac2c81e9092be67512611d2a/scripts/pi-hole/js/footer.js#L140-L143

This obviously leads to all classes being removed, along with their styling.
The more interesting question is: why does every other browser render the data table after these lines in footer.js? I honestly don't have a good answer.

Other pages don't cause this issue in Safari due to how the data fetching is implemented (an explicit initTable callback function which gets pushed onto the call stack). I considered using the same implementation here, but
in my opinion this wouldn't avoid the issue from occurring, because it is still somewhat of a race condition.

How does this PR accomplish the above?:

To avoid race conditions in general, I added an exception to the initialization of iCheck elements by using a :not selector for all elements that have an id attribute starting with "status_".

What documentation changes (if any) are needed to support this PR?:

none

@PromoFaux
Copy link
Member

@jfb-pihole, as you were able to reproduce the issue that this PR fixes - can you please check that this fixes it? Thanks!

@PromoFaux
Copy link
Member

Just to note, you wont be able to use pihole checkout web [branch] but you should be able to:

cd /var/www/html/admin
git checkout -b lionralfs-fix/1849-checkboxes devel
git pull https://github.com/lionralfs/AdminLTE.git fix/1849-checkboxes

@sbytnar
Copy link

sbytnar commented Nov 6, 2021

Hi. For this same bug, I use
.not('["data-toggle"="toggle"]'); // Skip bootstrapToggle which has its own theme.
instead of
.not("[id^=status_]")

Approvers... please do consider merging this PR as-is though. EIther way, this modification fixes the #1849 problem.

@yubiuser
Copy link
Member

yubiuser commented Nov 9, 2021

I checked, `id="status_" does only occur three times, all on the group management pages.

Bildschirmfoto zu 2021-11-09 20-32-34

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 had a similar issue on iPadOS 15.1 both on Safari and Chrome. The would not display the relevant tables to all. This PR fixed the issue.

@yubiuser yubiuser merged commit 14d44f2 into pi-hole:devel Nov 9, 2021
@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/list-of-configured-groups-only-shows-a-green-square/51178/2

@lionralfs lionralfs deleted the fix/1849-checkboxes branch November 28, 2021 17:15
@DL6ER DL6ER mentioned this pull request Dec 22, 2021
@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/pi-hole-ftl-v5-12-web-v5-9-and-core-v5-7-released/51795/1

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.

Broken "Group management" view in Safari 14.1.1 Groups screen displays incorrectly in Safari 14.0.3
5 participants