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

Echarlie 32 bootstrap4 #53

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Echarlie 32 bootstrap4 #53

wants to merge 3 commits into from

Conversation

echarlie
Copy link
Member

@echarlie echarlie commented Dec 21, 2021

substantial rewrite of templates to support bootstrap 5. fixes #32

Major changes:

  • panel -> card
  • form-group eliminated. need weird stuff
  • lots more use of flex.
  • substantially changed semantics of nav
  • minor changes to other form stuff.

see https://getbootstrap.com/docs/4.6/migration/ and https://getbootstrap.com/docs/5.0/migration

Copy link
Member

@mutantmonkey mutantmonkey left a comment

Choose a reason for hiding this comment

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

I haven't tried this out in a browser yet, but I assume you have. It looks like there's one typo that should be fixed before we merge this.

BTW, we may want to evaluate whether or not it's appropriate to include the .map files and other Bootstrap files we aren't using. I'm fine with doing it, but it is going to lead to larger Docker images than necessary. For now, I'll leave that up to you.

@@ -1,15 +1,15 @@
{% extends "admin/base.html" %}
{% set page_title="Roles" %}
{% block nav_admin_roles %}<li class="active"><a href="{{ url_for('admin.roles') }}">Roles</a></li>{% endblock %}
{% block nav_admin_roles %}<li class="nav=item"><a class="nav-link active" href="{{ url_for('admin.roles') }}">Roles</a></li>{% endblock %}
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be nav-item?

Copy link
Member Author

Choose a reason for hiding this comment

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

yup. looks like it should be.

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.

Bootstrap 4
2 participants