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

add bootstrap5 templates #304

Merged
merged 1 commit into from
Aug 6, 2021
Merged

Conversation

PetrDlouhy
Copy link
Contributor

@PetrDlouhy PetrDlouhy commented Aug 5, 2021

I have added templates for Bootstrap 5. The only difference, that is really needed is renaming of data-toggle to data-bs-toggle.

But I also added the {{ extra_class}} attribute which I found very handy. It can be used this way:

{% with extra_class="flex-wrap flex-row" %}
   {% sitetree_menu from "footer_3" include "trunk,topmenu" template "sitetree/menu_bootstrap5.html" %}
{% endwith %}

@coveralls
Copy link

coveralls commented Aug 5, 2021

Coverage Status

Coverage remained the same at 95.827% when pulling d3fe2d4 on PetrDlouhy:bootstrap5 into 8685369 on idlesign:master.

@idlesign
Copy link
Owner

idlesign commented Aug 5, 2021

Thank you.

This class thing could be useful indeed, so I'd like to ask you also to update the docs: to mention this extra class and new templates. https://github.com/idlesign/django-sitetree/blob/master/docs/source/templatesmod.rst

And another thing: it'd probably more flexible if we call that extra_class_ul.
This way we'll be able to add extra_class_li etc. in future if decided.

@@ -14,6 +14,12 @@ Nevertheless pay attention that menu template also uses two CSS classes marking
* **current_item** — marks item in the tree, corresponding to current page;
* **current_branch** — marks all ancestors of current item, and current item itself.

If needed, you can set estra CSS classes to the *ul* element with `extra_class_ul` variable. For example::
Copy link
Owner

Choose a reason for hiding this comment

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

Typo here — extra.
Also missing the new templates listing.

@PetrDlouhy
Copy link
Contributor Author

@idlesign I have changed the PR to your suggestions.

BTW There is 1 more thing, for which I have to override the whole template. I am using font-awesome icons, so I need to make the item.title_resolved safe (see #91 (comment)). I don't think, that is wise to add something like that to templates generally (in some systems users could have access to the sitetree editing). But I can either do something like

{% block title %}{{ item.title_resolved }}{% endblock %}

so that only this part of the template needs to be overriden, or it can be done by some settings (like SITETREE_FORCE_SAFE=True).
What do you think is better?

@idlesign
Copy link
Owner

idlesign commented Aug 6, 2021

@idlesign I have changed the PR to your suggestions.

Thank you. There's the pending issue though — #304 (comment)

I don't think, that is wise to add something like that to templates generally (in some systems users could have access to the sitetree editing).

This has no connection to Bootstrap template. Let's open another issue to discuss it, if it's required. Yet, I'd rather try to play with mark_safe() somehow before anything else.

@PetrDlouhy
Copy link
Contributor Author

@idlesign Sorry about the pending issue - I didn't push it properly yesterday.

The issue regarding HTML in title_resolved is here: #305

@idlesign idlesign merged commit 7a1a429 into idlesign:master Aug 6, 2021
@idlesign
Copy link
Owner

idlesign commented Aug 6, 2021

Thank you. Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants