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

theme: add config for instance theme file #59

Merged
merged 1 commit into from
Feb 6, 2020

Conversation

zzacharo
Copy link
Member

No description provided.

from flask_webpackext import WebpackBundle

theme = WebpackBundle(
__name__,
'assets',
entry={
'invenio-app-rdm-theme': './scss/invenio_app_rdm/theme.scss',
'invenio-app-rdm-theme': os.path.join('./scss/instance_theme', current_app.config.get(
'INSTANCE_THEME_FILE', './scss/invenio_app_rdm/theme.scss')),
Copy link
Contributor

@fenekku fenekku Jan 28, 2020

Choose a reason for hiding this comment

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

Obviously depends on the application context issue resolution. But also:

Normal: This results in ''./scss/instance_theme/./scss/invenio_app_rdm/theme.scss'. What about this:

'invenio-app-rdm-theme': current_app.config.get('INSTANCE_THEME_SCSS', './scss/invenio_app_rdm/theme.scss')

where INSTANCE_THEME_SCSS is a full path to the scss file? Or breaking it up into INSTANCE_THEME_SCSS_DIR and INSTANCE_THEME_SCSS_FILE so the same pattern as for template files (js files too?) can be used.

In any case we should also add INSTANCE_THEME_SCSS (or the other configuration variables) to the config.py so that this configuration variable is documented.

Copy link
Member Author

Choose a reason for hiding this comment

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

@fenekku you are right, my intention was to create the fixed instance_theme folder only if the config variable INSTANCE_THEME_FILE existed otherwise to fallback to './scss/invenio_app_rdm/theme.scss'. Also I wanted to hide as much of this from the end user and not have to oblige a specific structure as in assets approach. But as you noticed what I am doing now is wrong but I would apply probably do is calculating if the config variable is there and then apply the .scss/instance_theme prefix otherwise fallback to the
'./scss/invenio_app_rdm/theme.scss'. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm... checking if the config variable is there and then adding the prefix if it is prevents us from using the config variable ourselves as the default...

What about hiding/auto-prefixing the scss part and having users and libraries need to namespace (pretty much like the templates case): instance_theme/my_theme.scss or invenio_app_rdm/theme.scss should be passed?

@lnielsen lnielsen merged commit 37e42be into inveniosoftware:master Feb 6, 2020
@lnielsen lnielsen mentioned this pull request Feb 14, 2020
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.

4 participants