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

CSS refactor #312

Merged
merged 48 commits into from
May 13, 2024
Merged

CSS refactor #312

merged 48 commits into from
May 13, 2024

Conversation

pierrotsmnrd
Copy link
Contributor

@pierrotsmnrd pierrotsmnrd commented Feb 3, 2024

Work in progress - do not merge Ready for review

This PR uses a better approach for CSS styling in Panel.

The method we used so far showed its limitations : too much CSS code inside the python code, preventing CSS code completion, linting breaking indentation, hard to maintain.

The new method relies on design modifiers.

  • A modifier allows to register a CSS file or raw CSS code, for a given panel widget. eg :
# modifies the `stylesheets` property of *all* buttons, to add one css file
pn.theme.fast.Fast.modifiers[pn.widgets.Button] = {
"stylesheets":["css/button.css"]
}
  • One key thing then is to use css class (using css_classes=['my_class_name', ]). Imagine we have 2 kinds of buttons, A and B, we want to style in a different way. We can add them css class my_button_A and my_button_B, do the following :
pn.theme.fast.Fast.modifiers[pn.widgets.Button] = {
"stylesheets":["css/button_A.css", "css/button_B.css"]
}

and each css file should contain rules starting with :

:host(.my_button_A) { 
  /* Rules for button type A*/
}

and similarly for button B

This PR applies to method to Ragna UI.
Currently WIP, the auth page and part of the main view are handled.

Caveat : not all CSS can be removed from python code.

  • some CSS rules need to be built from python variables ( see central_view.py:L307)
  • some widgets seem immune to modifiers, depending on how they're built. For instance, all the widgets from the ChatInterface like ChatMessage, and "source info" buttons, cannot be styled with modifiers. I think it's a bug in Panel and I need to dig to identify the root cause. Eventually every widget should be modifiable using modifiers.

Copy link
Member

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

some CSS rules need to be built from python variables ( see central_view.py:L307)

For these, could you use the functionality I introduced #257?

some widgets seem immune to modifiers, depending on how they're built. For instance, all the widgets from the ChatInterface like ChatMessage, and "source info" buttons, cannot be styled with modifiers. I think it's a bug in Panel and I need to dig to identify the root cause. Eventually every widget should be modifiable using modifiers.

I believe this was fixed in holoviz/panel#6304. Should we wait for this to be released before we merge this PR?

@pierrotsmnrd
Copy link
Contributor Author

pierrotsmnrd commented Feb 5, 2024

For these, could you use the functionality I introduced #257?

Yes that's what I'm doing, see here

I believe this was fixed in holoviz/panel#6304. Should we wait for this to be released before we merge this PR?

I confirm this fixes the issue. Yes, we will have to wait for it to be released before merging.

Copy link
Member

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

Thanks @pierrotsmnrd, this is awesome! This resolves most of my gripes that I had with the way we styled the UI. I left a bunch of comments below. Two higher-level comments:

  1. I see the styles.py file still has massive CSS stylesheets. Can they be moved to CSS files as well?

  2. Since we now only rarely need to use ui.stylesheets (basically only if the style is only known at runtime), should we simplify the helper function? I'm thinking of it only accepting one class selector at a time. That would turn

    stylesheets=ui.stylesheets(("class-selector", {...}))

    into

    stylesheets=[ui.stylesheet("class-selector", {...})]

    Not a massive improvement, but I think this is easier to read. If you want to go for this in this PR, here is the new definition:

    def stylesheet(
        selectors: Union[str, Iterable[str]], declarations: dict[str, str]
    ) -> str:
        return "\n".join(
            [
                f"{selectors if isinstance(selectors, str) else ', '.join(selectors)} {{",
                *[f"    {property}: {value};" for property, value in declarations.items()],
                "}",
            ]
        )

    I can also send a follow-up PR if you prefer that.

ragna/deploy/_ui/app.py Outdated Show resolved Hide resolved
ragna/deploy/_ui/app.py Outdated Show resolved Hide resolved
ragna/deploy/_ui/auth_page.py Outdated Show resolved Hide resolved
ragna/deploy/_ui/central_view.py Outdated Show resolved Hide resolved
ragna/deploy/_ui/central_view.py Show resolved Hide resolved
ragna/deploy/_ui/styles.py Outdated Show resolved Hide resolved
ragna/deploy/_ui/styles.py Outdated Show resolved Hide resolved
ragna/deploy/_ui/styles.py Outdated Show resolved Hide resolved
ragna/deploy/_ui/styles.py Outdated Show resolved Hide resolved
@pmeier pmeier linked an issue Feb 7, 2024 that may be closed by this pull request
@pierrotsmnrd
Copy link
Contributor Author

pierrotsmnrd commented Feb 7, 2024

I see the styles.py file still has massive CSS stylesheets. Can they be moved to CSS files as well?

The goal is to get rid of all the CSS currently in python files and move that in proper CSS files. So yes, they'll be moved.

Since we now only rarely need to use ui.stylesheets (basically only if the style is only known at runtime), should we simplify the helper function? I'm thinking of it only accepting one class selector at a time.

Totally ok with the concept, but I suggest to name it ui.css, it's shorter but conveys the same meaning.

Copy link
Member

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

I didn't review everything yet, but I already found a few differences in the UI styling:

Create chat

main

Screenshot from 2024-02-12 15-57-16

PR

Screenshot from 2024-02-12 15-56-02

Message feed

main

Screenshot from 2024-02-12 15-57-40

The divider is black on the PR while it is a lighter gray on main.

PR

Screenshot from 2024-02-12 15-56-43

The messages on the PR are missing the border / background color.

ragna/deploy/_ui/modal_configuration.py Outdated Show resolved Hide resolved
ragna/deploy/_ui/styles.py Outdated Show resolved Hide resolved
ragna/deploy/_ui/styles.py Outdated Show resolved Hide resolved
ragna/deploy/_ui/styles.py Outdated Show resolved Hide resolved
pierrotsmnrd and others added 2 commits February 20, 2024 07:00
Co-authored-by: Philip Meier <github.pmeier@posteo.de>
@pierrotsmnrd
Copy link
Contributor Author

There was indeed a problem with the separator in the configuration modal.
I don't reproduce your screenshot of the chat feed though. I guess it's a matter of emptying cache. Can you try that ?

@pmeier
Copy link
Member

pmeier commented Mar 28, 2024

@pierrotsmnrd panel==1.4.0 (tracked in #379) also introduced new ways of styling the chat messages: https://blog.holoviz.org/posts/panel_release_1.4/#other-enhancements Can this be adopted here to simplify stuff?

@pmeier pmeier mentioned this pull request Apr 2, 2024
@pmeier pmeier added this to the 0.3.0 milestone Apr 24, 2024
commit 04168ab
Author: Arjun Verma <arjunverma.oc@gmail.com>
Date:   Wed May 1 12:35:41 2024 +0530

    #373 Include documentation_helpers in module (#395)

    Co-authored-by: Philip Meier <github.pmeier@posteo.de>

commit f947f2d
Author: Philip Meier <github.pmeier@posteo.de>
Date:   Mon Apr 29 17:08:40 2024 +0200

    bump mypy to 1.10 (#398)

commit e0fe014
Author: Kevin Klein <7267523+kklein@users.noreply.github.com>
Date:   Mon Apr 29 12:52:49 2024 +0200

    Add instructions to install from conda-forge. (#396)

    Co-authored-by: Philip Meier <github.pmeier@posteo.de>

commit 7963453
Author: Philip Meier <github.pmeier@posteo.de>
Date:   Thu Apr 25 20:38:50 2024 +0200

    use custom JSON type for database for more generic support (#389)

commit 3a5b82d
Author: William Black <125844868+smokestacklightnin@users.noreply.github.com>
Date:   Wed Apr 24 01:37:48 2024 -0700

    Remove Mosaic Assistant (#387)
@pierrotsmnrd pierrotsmnrd linked an issue May 13, 2024 that may be closed by this pull request
@pmeier
Copy link
Member

pmeier commented May 13, 2024

image

"Source info" button looks different from the copy button. This has nothing to do with panel==1.4 as it is visible in #312 (review) as well.

@pierrotsmnrd
Copy link
Contributor Author

Capture d’écran 2024-05-13 à 12 40 10

This is what I see. Can you make sure you empty cache before refresh ?

@pmeier
Copy link
Member

pmeier commented May 13, 2024

I'm seeing the issue on Firefox and Chromium both with cleared caches.

Copy link
Member

@pmeier pmeier 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 ton Pierre! This looks awesome!

@pierrotsmnrd pierrotsmnrd merged commit 6f7fbac into main May 13, 2024
12 checks passed
@pierrotsmnrd pierrotsmnrd deleted the css_refactor branch May 13, 2024 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bump panel to 1.4.0 Improve stylessheets formatting in UI code
2 participants