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

templates: pass ui classes through macro parameters #43

Merged
merged 1 commit into from
Jan 22, 2025

Conversation

kpsherva
Copy link
Contributor

@kpsherva kpsherva commented Jan 22, 2025

Attention (for CDS-RDM)

  1. we need to backport it to v3

Copy link
Contributor

@jrcastro2 jrcastro2 left a comment

Choose a reason for hiding this comment

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

LGTM! Just a very minor comment.

@@ -20,7 +20,7 @@ def get_active_banners_for_request():

def style_category(category):
Copy link
Contributor

Choose a reason for hiding this comment

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

Very minor: Since there is no more styling we could simplify this to something like (styles could be a conf variable as well, if someone would like to customize it):

def style_category(category):
    """Return predefined Semantic-UI classes for each banner category."""
    styles = {
        "warning": "warning",
        "other": "grey",
    }
    return styles.get(category, "info")

Copy link
Member

@slint slint Jan 22, 2025

Choose a reason for hiding this comment

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

Agree, and I would also avoid the .format(...), and just go with early returns, we're not doing anything extra with the string tempalte.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we could in general rethink how this works and it is a bigger subject of mixing the backend with UI params. Please merge if you consider this good enough

@jrcastro2 jrcastro2 merged commit fff1156 into inveniosoftware:master Jan 22, 2025
2 checks passed
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.

Featured communities: CSS issues
4 participants