Skip to content

Replace bootstrap-notify.js by bootstrapv5-toast - #3843

Open
yubiuser wants to merge 10 commits into
developmentfrom
replace/notify
Open

Replace bootstrap-notify.js by bootstrapv5-toast#3843
yubiuser wants to merge 10 commits into
developmentfrom
replace/notify

Conversation

@yubiuser

@yubiuser yubiuser commented Aug 6, 2026

Copy link
Copy Markdown
Member

What does this PR aim to accomplish?:

Replaces bootstrap-notify.js with bootstrap-5 toasts. bootstrap-notify is unmaintained and not compatible with jquery4 (see #3842)

The flow is the following: we create a toast-container which is re-usable and can hold multiple toasts. On showAlert() a new toast is created inside the container. Bootstrap handles the delay and autohide by itself.


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

@yubiuser yubiuser mentioned this pull request Aug 6, 2026
1 task
@yubiuser
yubiuser force-pushed the replace/notify branch 2 times, most recently from d72ed22 to ed6cf23 Compare August 6, 2026 17:44
Signed-off-by: yubiuser <github@yubiuser.dev>
@yubiuser
yubiuser force-pushed the replace/notify branch 2 times, most recently from fc6e68c to de6b61c Compare August 6, 2026 18:31
@rdwebdesign

Copy link
Copy Markdown
Member

When you squash the commits, can you please add a commit message explaining the changes?

Signed-off-by: yubiuser <github@yubiuser.dev>
…and was used only in two places

Signed-off-by: yubiuser <github@yubiuser.dev>
Signed-off-by: yubiuser <github@yubiuser.dev>
@yubiuser
yubiuser marked this pull request as ready for review August 7, 2026 07:10
@yubiuser
yubiuser requested a review from a team as a code owner August 7, 2026 07:10
@yubiuser

yubiuser commented Aug 7, 2026

Copy link
Copy Markdown
Member Author

OK, it's in a state ready for review/comments.
After internal discussion, I removed the option to update existing toasts, because it added a lot of complex code and was only used in two places (deleting messages and deleting DHCP leases).

Signed-off-by: yubiuser <github@yubiuser.dev>
Signed-off-by: yubiuser <github@yubiuser.dev>
Signed-off-by: yubiuser <github@yubiuser.dev>
@yubiuser
yubiuser force-pushed the replace/notify branch 4 times, most recently from 56b1489 to 97bed99 Compare August 7, 2026 19:52
Signed-off-by: yubiuser <github@yubiuser.dev>
Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
@rdwebdesign

Copy link
Copy Markdown
Member

I tested and it is working as expected, but I changed a few things:

  • I removed the small margin-top from the icon. That margin was resulting in a slightly misaligned icon (it was closer to the bottom line - see images).
  • the code inherited from the original function puts the title inside a <strong> element, but the new code already puts the title in a <strong> element, resulting in a font-weight heavier than "bold" (see images). I removed the tag.
  • I also removed the leading non-breaking space and the final line break the title (not necessary).
image

 image

@yubiuser

yubiuser commented Aug 8, 2026

Copy link
Copy Markdown
Member Author

Thanks. I'd like to add two things here before this should be merged: implement mass-delete for the messages table (notification currently breaks there) and I'm thinking about moving the handling of the toast state from globalThis into show alert.

@DL6ER DL6ER left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

messages.js and settings-dhcp.js used to key their toasts per item (toasts[id], toasts[ip]), both now write to a single global slot. delMsg() and delLease() run in a loop from "Delete Selected", so deleting five rows creates five info toasts but every response updates only the last one.

showAlert() should not throw when the last argument is missing. It is our error-reporting path, so one missed call site turns a reported API error into an uncaught exception, and nothing lints for it. Default to null instead.

settings.js and settings-dns.js pass null as the message. It renders empty today, but #3847 skips the body only for an empty string. escapeHtml(message ?? "") covers both.

The toast container is declared twice, in header_authenticated.lp and in getOrCreateToastContainer().

On #3847: the --bs-toast-bg override drops AdminLTE's subtle body for a solid one - intentional?

@rdwebdesign

Copy link
Copy Markdown
Member

On 3847: the --bs-toast-bg override drops AdminLTE's subtle body for a solid one - intentional?

Yes.
That is intentional to match the same visual we currently have in master, but using the suggested classes, the toast styles can be changed per theme.

@DL6ER

DL6ER commented Aug 9, 2026

Copy link
Copy Markdown
Member

Moving the toast state into showAlert() sounds like the better route. Sketch, if it helps:

const toastRegistry = new Map();

function showAlert(type, icon, title, message, key = null) {
  // ...
  const oldToastInstance = key === null ? null : toastRegistry.get(key);
  // ...
  if (key !== null) {
    toastRegistry.set(key, toastInstance);
  }

  // next to the dispose() in the "hidden.bs.toast" handler
  if (key !== null && toastRegistry.get(key) === toastInstance) {
    toastRegistry.delete(key);
  }
}

Callers pass a key instead of an instance, e.g., "delLease-" + ip. That fixes the mass-delete case by construction and takes the ten globalThis.toasts ??= {} lines and the mandatory last argument with it.

It is 60 lines across 12 files though, and the utils.js half on its own would silently stop toasts from being replaced, so it is all or nothing. If you would rather not touch that much: I have counted this and a working demo which I could push onto replace/notify, or we keep the current shape and only key the two affected maps per item ("deleteMessage" + id, "delLease" + ip), which is four lines in each of the two files.

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