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

Vanilla Macro loader example (DO NOT MERGE) #14209

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pastelcyborg
Copy link
Contributor

@pastelcyborg pastelcyborg commented Aug 20, 2024

This PR exists solely to demonstrate examples and experiments for loading Vanilla Macros from node_modules/ using Flask/Jinja. A permanent solution will likely be integrated into Flask Base directly.

DO NOT MERGE.

@webteam-app
Copy link

Copy link

codecov bot commented Aug 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.51%. Comparing base (f8e9844) to head (8d5943a).
Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14209      +/-   ##
==========================================
- Coverage   69.57%   69.51%   -0.06%     
==========================================
  Files         120      120              
  Lines        3412     3412              
  Branches     1172     1172              
==========================================
- Hits         2374     2372       -2     
- Misses       1013     1015       +2     
  Partials       25       25              

see 1 file with indirect coverage changes

Copy link
Member

@jmuzina jmuzina left a comment

Choose a reason for hiding this comment

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

This is much simpler than I thought it would be!

templates/desktop/index.html Show resolved Hide resolved
@britneywwc
Copy link
Contributor

This is great, thank you! :)

I tried running the example on demo and it's throwing 500, running it locally shows that the macros are not found.
Screenshot 2024-08-22 at 10 32 23 AM

Upon checking, there are no templates/ files under node_modules/vanilla_framework. Is this soon to be added from Vanilla's side?

@pastelcyborg
Copy link
Contributor Author

This is great, thank you! :)

I tried running the example on demo and it's throwing 500, running it locally shows that the macros are not found. Screenshot 2024-08-22 at 10 32 23 AM

Upon checking, there are no templates/ files under node_modules/vanilla_framework. Is this soon to be added from Vanilla's side?

Vanilla in this project would need to be upgraded to at least 4.15.0 to get the patterns.

@pastelcyborg
Copy link
Contributor Author

@britneywwc Actually, a few other minor changes will need to be made to Vanilla as well, because the macros directory is not currently published as part of the npm package, so unfortunately this server change won't be useful until those changes are made. Sorry about that!

Comment on lines +175 to +178
loader = ChoiceLoader([
FileSystemLoader('templates'),
FileSystemLoader('node_modules/vanilla-framework/templates/')
])
Copy link
Member

Choose a reason for hiding this comment

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

What happens if a project has a file in templates with the exact same file path as a macro we publish that they'd like to use?

My assumption is the project's template will be used since it's listed first here. However this means we are slightly restricting the file structure of projects that use flask base.

This isn't a bad thing, but maybe we should add some logic that outputs warnings if any identical file-paths are found between these two directories?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I'd been wondering the same thing - maybe the filenames of our macros should be namespaced somehow (vf.tiered_list.jinja or whatever)?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, I'd been wondering the same thing - maybe the filenames of our macros should be namespaced somehow (vf.tiered_list.jinja or whatever)?

I'd support that.

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.

4 participants