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 basic support for themes. Theme can be configured in config file … #8

Merged
merged 3 commits into from
Nov 4, 2018

Conversation

mwallraf
Copy link

@mwallraf mwallraf commented Nov 1, 2018

…or directly in url_for()

Hi, as discussed earlier, here's the PR to include support for themes in flask-gentelella.

If no theme is provided in the config file then nothing changes, if a theme name is configured then all static files should exist in /static/theme_name/ (in all blueprints)
Or it's possible to include a theme for each individual url_for('static', filename='', theme='') request.

I've included a sample 'theme_dark' theme which is exactly the same as the original but with a dark default background.

@afourmy afourmy self-requested a review November 1, 2018 10:04
@afourmy
Copy link
Owner

afourmy commented Nov 1, 2018

The problem is that it doesn't scale: you're adding 4K duplicated files (2M lines of code - almost all the project in fact) per theme.

Travis complains because the files in the theme folder are not ignored by ESlint (that's what causes the build to fail).

Can you update url_for to default to the static folder in case a file is not found in the theme folder, such that if you add a theme that changes the background color, you'll need to add only that one file to the theme folder ?

Thanks

@coveralls
Copy link

coveralls commented Nov 1, 2018

Pull Request Test Coverage Report for Build 108

  • 11 of 14 (78.57%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.4%) to 95.213%

Changes Missing Coverage Covered Lines Changed/Added Lines %
app/init.py 11 14 78.57%
Totals Coverage Status
Change from base Build 105: -1.4%
Covered Lines: 179
Relevant Lines: 188

💛 - Coveralls

@mwallraf
Copy link
Author

mwallraf commented Nov 1, 2018

Actually my first idea was to use themes only for files containing img or css in the filename but this would require too many changes and it could have unwanted behaviour for vendor modules etc.
And I wasn't exactly sure if it's a good idea to check for the existence of each file for each request so I settled on duplicating the files.

Or perhaps it's better to give the developer the choice to use a theme or not based on each individual url_for() request?
Example:
url_for() by default will not use any themes
url_for(.., use_theme=True) will use the globally configured theme (if configured)
url_for(.., theme='mytheme') will use mytheme

Thanks for the tip for Travis.

@mwallraf
Copy link
Author

mwallraf commented Nov 1, 2018

After doing some tests your suggestion seems to work fine: a check is first done if the file in the theme folder exists, if not then the default location is used.
The example /static/themes/dark/ just contains the updated CSS file now.
If no theme is applied then no file check is being done.

@afourmy afourmy merged commit b8d8c39 into afourmy:master Nov 4, 2018
@afourmy
Copy link
Owner

afourmy commented Nov 4, 2018

Ok merged, thanks.

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.

3 participants