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

Migrate to Bootstrap 4 #12

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

Conversation

notjosh
Copy link
Collaborator

@notjosh notjosh commented Oct 11, 2019

The ongoing modernisation of the project continues!

In order to bump some of the designs, I figured it made sense to bump us to the latest Bootstrap. Because of Computers™, that means swapping Flask-Bootstrap from bootstrap-flask, and migrating a bunch of styles.

I added prettier support for the HTML, though some of the support is a little quirky, because it's Jinja, not HTML. There's an open issue for full support, but I don't think it's a showstopper for now. I'll take "consistent and pretty good" over "perfect" formatting.

I also removed the "face" support in the preview screen since it wasn't actually being used.

@notjosh
Copy link
Collaborator Author

notjosh commented Oct 11, 2019

Some screenshots, to save you spinning it up:

Screen Bootstrap 3 Bootstrap 4
Home image image
About image image
Printer list image image
Printer detail image image
Print preview image image


{% block page_content %}
{% extends "base.html" %} {% block title %}Sirius - Admin{% endblock %} {% block
page_content %}
Copy link
Member

Choose a reason for hiding this comment

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

This formatting looks a little broken to me.... does Jinja allow a line break in a directive? Also, it would be much preferred to put the extends directive on a new line, if at all possible

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Jinja allows the line breaks, but Prettier is only parsing/formatting it as HTML. There's a(n open) ticket for Jinja support (prettier/prettier#5754), and a link to a Nunchuks based plugin (which is incomplete).

From what I can tell, any of the Python specific formatters (black, autopep8, yapf) won't touch the HTML part of Jinja, and Prettier won't do much to consider the Python parts (just format them as blobs). Given that there's more HTML than Python here, I'm more inclined to favour getting the HTML readable.

We could manually format the Jinja specific lines and throw in <!-- prettier-ignore -->, but that might just be more trouble than it's worth?

I'll take a look at the Nunchucks plugin - it might have enough of the simple functionality to at least format the Jinja blocks sequentially, instead of a single line.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fwiw the Nunchuks plugin has some legs, but also has some bugs. It'll autoformat that block as:

image

/me adds it to the list

Copy link
Member

Choose a reason for hiding this comment

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

That looks pretty good to me!

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.

None yet

2 participants