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

Feature/plotly theming #6

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Feature/plotly theming #6

wants to merge 21 commits into from

Conversation

GedionT
Copy link
Member

@GedionT GedionT commented Oct 14, 2024

Summary

This request mainly adds theming functionality for Plotly charts by adding a custom theme to Plotly and a prebuilt wrapper for a few plot types. A new integration page is also introduced to the demo application to showcase the new Plotly theme feature.

Furthermore, this request addresses the link animation on-hover effect (#4), visited URL style, and checkbox state issue (#5 (comment)).

New Features

  • Added a custom Plotly theme
  • Added UNDP-themed color schemes
  • An integration demo page to showcase Plotly charts with custom themes
  • Added an expander component in the text tab on the standard page

Bug Fixes

  • Fixed checkbox issue that used to automatically trigger checked state on mouse enter event
  • Fixed link animation effect on hover
  • Fixed visited link style from red to standard grey color

Checklist

  • I have read the contributing guidelines.
  • My code follows the style guidelines of this project.
  • I have performed a self-review of my code.
  • I have commented on my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added the required dependencies to pyproject.toml and updated poetry.lock (if applicable).
  • I have updated the documentation (if applicable).
  • I have included any necessary screenshots or examples of the new functionality.

Screenshots / Examples

image

Additional Notes

None

@GedionT GedionT self-assigned this Oct 21, 2024
Copy link
Contributor

@mykolaskrynnyk mykolaskrynnyk left a comment

Choose a reason for hiding this comment

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

Let's keep the package scoped to Streamlit with plotly theming being a small extra, rather than trying to provide plotting-like functionality. We also don't need to store large dictionaries, these can go to data folder as JSONs. Some code parts can be significantly improved. See the comments elsewhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is overly verbose. Some of the keys in undp_colors are used only once (or not at all).

  • Write the configuration explicitly in the dictionary inside custom_template.layout.update (even if this requires repetition).
  • Remove undp_colours.
  • Move the template dictionary to data/ as a JSON file, them read it back here with utils.read_file.
  • Call the template "undp" instead of "undp_template".

Copy link
Contributor

Choose a reason for hiding this comment

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

Providing wrappers over plotly is beyond the scope of st-undp. It is limiting in terms of customisation and not particularly usable due to the required data format imposed by these functions (most data will have columns like "country" and "value" rather than "x" and "y", so users will have to rename columns to use these functions or the functions need to be extended to take variable names, either way, it is workable).

I suggest to take 5-7 functions from here and include those as concrete examples in pages within the app as a showcase. This module can then be removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Add brief comments to explain what the changes are intended for.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • Once plotly_components is removed, import the required function from plotly_theme explicitly.
  • Applying the theme by default like this is not desirable. Remove pio.templates.default = "undp_template" from plotly_theme.py. This will make the function define the undp theme but not set it. Then, enable users to set the theme via apply_style. Defining the theme would also allow users to set it explicitly later on (outside of apply_style).

Copy link
Contributor

Choose a reason for hiding this comment

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

define_plotly_theme()
if style_plotly:
    set_plotly_theme()

Copy link
Contributor

Choose a reason for hiding this comment

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

A new line at the end is missing and code formatting is inconsistent. Use make format.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • As mentioned elsewhere, keep only some of the examples below and move the plotly code needed to produce them in here.
  • This module is highly repetitive. You don't need to define the inputs within each tab. Instead, create a callable and make it return the selected values. Define a key with a prefix. See the example below.
def insert_controls(prefix: str) -> dict:
    controls = {
        "title": st.text_input(
            "Chart Title", f"{prefix.capitalize()} Title", key=f"{prefix}_title"
        ),
        "x_title": st.text_input("X Axis Title", "X Axis", key=f"{prefix}_x_title"),
        "y_title": st.text_input("Y Axis Title", "Y Axis", key=f"{prefix}_y_title"),
        "annotation": st.text_input(
            "Annotation (e.g., Source)",
            "Source: Example Data",
            key=f"{prefix}_annotation",
        ),
        "annotation_x": st.number_input(
            "Annotation X Position (0 to 1)",
            min_value=0.0,
            max_value=1.0,
            value=0.0,
            key=f"{prefix}_annotation_x",
        ),
        "annotation_y": st.number_input(
            "Annotation Y Position (0 to 1)",
            min_value=-1.0,
            max_value=1.0,
            value=-0.3,
            key=f"{prefix}_annotation_y",
        ),
    }
    return controls


# ...
with tabs[1]:
    col1, col2 = st.columns(2)
    with col1:
        controls = insert_controls("area")
    with col2:
        area_data = {"x": [1, 2, 3, 4, 5], "y": [5, 10, 15, 20, 25]}
        st_undp.area_chart(area_data, **controls)

It's not really "integrations", more like "extras" or "add-ons".

Copy link
Contributor

Choose a reason for hiding this comment

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

If name similarity is an issue, modify the names, e.g., code -> code_str, code_text or even body (as per st.code signature), so st.code(body) but don't use uppercase for variables you redefine.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is more like "extras", not so much "integrations".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants