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

Find a way to make the html for Bootstrap alerts persistent, and avoid putting HTML code in uiUtil.js #470

Closed
Jaifroid opened this issue Jan 13, 2019 · 3 comments
Assignees
Milestone

Comments

@Jaifroid
Copy link
Member

In line with https://github.com/kiwix/kiwix-js/pull/467/files#r247345420, we need to find a way to make Bootstrap alerts remain in the DOM (and hide them when not shown) rather than allowing Bootstrap to remove them from the DOM (which is the native Bootstrap behaviour). This avoids having to re-create the alert in JavaScript each time we wish to display it.

@Jaifroid Jaifroid added this to the v2.6 milestone Jan 13, 2019
@Jaifroid Jaifroid self-assigned this Jan 13, 2019
@mossroy mossroy changed the title Find a way to make the html for Bootstrap alerts persistent Find a way to make the html for Bootstrap alerts persistent, and avoid putting HTML code in uiUtil.js Mar 2, 2019
@mossroy
Copy link
Contributor

mossroy commented Mar 2, 2019

More generally, we should not generate HTML strings in javascript code. It makes maintenance complicated (for example if we re-design the UI)

@Jaifroid
Copy link
Member Author

@mossroy, you may have missed it, but in #527 I pointed out that this issue could be closed very easily by adopting one of the commits from that PR, and it may be worth doing it for #525 release. I copy the comment below for convenience:

FYI Commit c463ba4 is a proposed fix for #470, i.e., we no longer have to create the alert boxes' html in code each time we want to display them.

However, as you can see, it doesn't save any code; on the contrary, it adds to the amount of code, because we have to implement an override on Bootstrap's own .alert('close') function. The only advantage of this code is that it maintains the separation between html and code, but at the cost of a little bit more complexity.

This commit (as opposed to the whole PR) could be added to a separate branch and we could have a PR on it as a solution to #470, if you'd prefer to do it that way, @mossroy . Just let me know, and also let me know if you think this way of doing it is better than what it replaces (just focus on commit c463ba4).

@Jaifroid
Copy link
Member Author

Jaifroid commented Mar 4, 2020

This issue was fixed in #527. Closing.

@Jaifroid Jaifroid closed this as completed Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants