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

Move template files from templates to templates/sqladmin #748

Merged
merged 1 commit into from
Apr 15, 2024

Conversation

hasansezertasan
Copy link
Contributor

Closes #747

I ran the test suite, looking good 🚀.
I "visually" tested it with the example code from Working with Custom Views - SQLAlchemy Admin, works fine ☮️.

@aminalaee aminalaee changed the title All files and folders in the "templates" directory are moved to the "templates/sqladmin" folder. Move template files from templates to templates/sqladmin Apr 15, 2024
Copy link
Owner

@aminalaee aminalaee left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the contribution 👍

@aminalaee aminalaee merged commit 9ed5414 into aminalaee:main Apr 15, 2024
5 checks passed
@aminalaee aminalaee mentioned this pull request May 13, 2024
@hasansezertasan hasansezertasan deleted the templates branch May 14, 2024 00:41
@lesleslie
Copy link

First, thanks all. I appreciate you.

I understand why it may be desirable to move templates to a 'templates/sqladmin' folder, but am curious why you just don't initialize the package loader with the 'templates/sqladmin' like so:

def init_templating_engine(self) -> Jinja2Templates:
        templates = Jinja2Templates("templates")
        loaders = [
            FileSystemLoader(self.templates_dir),
            PackageLoader("sqladmin", "templates/sqladmin"),
        ]

then you wouldn't have to add the 'sqladmin' to the extends. For instance in index.html:

{% extends "sqladmin/layout.html" %}

My point being, that if I have a custom 'layout.html', or other html file, that I want shared between sqladmin and another application (a directory that doesn't start with 'sqladmin') I can't do that then without overriding every other template file in the package that references 'sqladmin/layout.html'. But when the index.html file extends is as such:

{% extends "layout.html" %}

the package loader would still be able to find 'templates/sqladmin/layout.html' but other loaders (in priority of choice before the package loader) would also be able to load a custom version of the file in whatever directories are in the loader's searchpath (ie 'templates/something_else/layout.html') without having to modify all of the other core template files accessed by the package loader.

I feel the way this is implemented actually hinders customization, and reusability, of templates because if you want to use a single custom template that is not in a 'sqladmin' directory (possible because it's shared with other applications), you have to then modify/override every other template in the package that references it - and then manually edit each one of those every time changes are made to the sqladmin codebase or default templates when you could just be using the default package templates instead.

I think this should be re-opened for discussion. Please let me know if this makes sense or if there is anything else I can do or provide to clarify my point.

Thanks again.

@hasansezertasan
Copy link
Contributor Author

hasansezertasan commented May 24, 2024

but am curious why you just don't initialize the package loader with the 'templates/sqladmin'

If you check out the problem at #747, you'll understand why we went in that direction.

I can't do that then without overriding every other template file in the package that references 'sqladmin/layout.html'.

You are right, there are some problems but they are going to be resolved when they are real pain in the ass (like Flask Admin).

Here is a solution recommendation for your scenario: #762 (reply in thread)

I also want to point out that this idea comes from Flask Admin, which has proved its reusability end extendibility.

@lesleslie
Copy link

Thanks for the reply!

I love this project because it stays as true to flask-admin as it can.

After 'correctly' updating my custom admin code, directory structure, and templates, I have come to conclusion that I no longer have issues, and have made my peace, with this change.

@hasansezertasan
Copy link
Contributor Author

Thanks for the reply!

You're welcome 🙏.

I love this project because it stays as true to flask-admin as it can.

I totally agree, I'm working on a Starlette adoption of Flask Admin, let me know if you are interested in, I'll share a link.

After 'correctly' updating my custom admin code, directory structure, and templates, I have come to conclusion that I no longer have issues, and have made my peace, with this change.

😂, happy to hear that. @flask-admin people did a lot of good design decisions, years of experience right?. Even though I don't understand some parts of its source code, I left them as is. They might know better! 🚀🚀🚀.

@justatrade
Copy link

justatrade commented Sep 9, 2024

Thanks for your project, first of all!

I've faced an issue using a 0.19 version after 0.16.1. I could not just create templates folder, move there original package files and customize them a bit. Using templates_dir parameter doesn't help.

Is there any way to achieve such goals like localisation and customisation?

Thanks in advance.

@aminalaee
Copy link
Owner

I think I didn't fully get your question.
Why would you move original package files there? You should override them in your templates directory.

@justatrade
Copy link

Because I'd like to use them with minimal changes)
BTW, I found a solution, not sure if it's right but it works.

  1. admin = Admin(app, templates_dir="templates")
  2. Created templates folder with subfolder sqladmin
  3. Moved all the original html files from package to templates/sqladmin
  4. Changed all the paths to html files inside html files by adding "sqladmin/" to the left of the file name.

This works fine for me, and now I successfully override base files.

@aminalaee
Copy link
Owner

Yes that is correct. This was the intention of this PR to need sqladmin inside the templates. Feel free to update the docs if you feel like there's missing information.

@aminalaee
Copy link
Owner

I hope this clarifies what you said: #817

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.

sqladmin + jinja templates conflict index.html
4 participants