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

[Feat] Add Figure model and introduce KPI card functions #493

Merged
merged 79 commits into from
Jun 18, 2024

Conversation

huong-li-nguyen
Copy link
Contributor

@huong-li-nguyen huong-li-nguyen commented May 22, 2024

Description

Why introduce the Figure model?

This PR was initially intended to enable reactive Cards as we needed them for the KPI Cards. Still, several iterations and prototypes clarified that a higher-level model was required to allow any dash component to be reactive.

We've been hesitant to introduce this model as we first wanted to see if it is needed, so we started with lower-level models such as Table, Graph, AgGrid, and Card as they are also more declarative. However, in the future, we could subclass the Table, Graph, and AgGrid from the Figure, see Antony's comment here: #493 (review)

As you already see, the naming is not optimal, which was also one of the reasons why we didn't go with it straight away 😄 So if you strongly disagree with Figure as a name, please raise your opinion here, such that we can find a name that everyone can accept more or less: #493 (comment)

I'll split docs in a separate PR as this model needs proper documentation and probably lots of explanation, as its purpose is unclear from the naming alone. @stichbury I would probably need your help on that 👍

What does this Figure solve for?
Essentially, it will enable any dash component to be reactive to controls (not filter interactions, as this logic needs to live in any subclass implementation for now). This could have already been useful and a better approach when:

  • We've included the dash-mermaid chart into Vizro
  • We've included custom D3 charts into Vizro
  • We've helped our users make components reactive to controls that are just a combination of HTML divs
  • And now, when we wanted to include reactive Cards
  • Essentially including any other dash component that doesn't fall into Table, Graph, AgGrid and doesn't qualify for another sub-category (can't think of any right now anyways)

How did we get here from the KPI Card?

Ultimately, the KPI Cards are just implementations of CapturedCallables that go into Figure.
However, before we got here, I considered four different approaches and did prototypes for all of them.

  • If you are curious how 1 and 2 would have looked like: a1b1228
  • Approach 3 you can see if you take a look at the code at the time of this commit: 90dfcb3

1) Use a custom chart with go.Indicator and provide to vm.Graph

  • Most convenient way of creating a static and dynamic KPI card without any additional core core changes
  • Several drawbacks when it came to resizing custom styling that we decided to create our custom component instead
  • Suitable as a temporary solution only

2) Use a custom component

  • The best way to currently create a static KPI Card as it allows for complete flexibility in terms of styling
  • The biggest drawback is that the KPI Card cannot be dynamic (reactive to controls) due to our constraint of components only being reactive to controls when they have a figure attribute and a CapturedCallable is provided
  • Suitable as a temporary solution only

3) Extend the vm.Card model with a figure attribute

  • Possibility to create a static and dynamic KPI Card
  • Full flexibility in terms of styling
  • Full flexibility for the users in terms of extension by provision of CapturedCallable
  • We stuck with this one for a while, but it got messy with the current API and combining it with the pre-existing arguments text and href. While it was possible, there were several drawbacks to this approach:
    - Because the text and nav card were now provided as CapturedCallable, they started loading as well, which just wasn't nice UX as these components are static
    - The build, in the end, was not connected to the Card, and any intention to do that meant having to move the dbc.Card outer container from the CapturedCallable, which led to several issues given classNames needing to be provided
    - Given that the build was not connected to the Card anymore but could have been the higher-level model for any dash component that we might want to be reactive to controls, approach 4 made more sense ⬇️

4) Create a new component with a figure attribute

  • Possibility to create a static and dynamic KPI Card
  • Full flexibility in terms of styling
  • Full flexibility for the users in terms of extension by provision of CapturedCallable
  • Unlocks many other blockers, e.g., provision of custom d3 charts or other dash components that we want to react to controls without them having to fit into Table, AgGrid, or Graph

What will be our default KPI Cards?
I've created designs for 3 KPI Cards and aligned with @Joseph-Perkins that these will be the ones we want to offer by default. Users can still create their own KPI Cards by providing their own CapturedCallable. Colors, etc., might still change, but the overall structure remains the same:

  • Simple KPI Card (Only title + value)
  • KPI Card with Icon (title + value + icon)
  • KPI Card with reference value (title + value + reference value)
  • KPI Card with the reference value and icon (could be potentially done based on the three above)
Screenshot 2024-05-23 at 14 13 04

Notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

    • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
    • I certify that (a) this contribution is my original creation and (b) to the extent it is not my original creation, I am authorized to submit this contribution on behalf of the original creator(s) or their licensees.
    • I certify that the use of this contribution as authorized by the Apache 2.0 license does not violate the intellectual property rights of anyone else.
    • I have not referenced individuals, products or companies in any commits, directly or indirectly.
    • I have not added data or restricted code in any commits, directly or indirectly.

@huong-li-nguyen huong-li-nguyen self-assigned this May 30, 2024
@huong-li-nguyen huong-li-nguyen added the Feature Request 🤓 Issue contains a feature request label May 30, 2024
Copy link
Contributor

@petar-qb petar-qb left a comment

Choose a reason for hiding this comment

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

Magnificent work! The "Figure" is something that opens up a lot of new possibilities 🚀.

vizro-core/examples/_dev/app.py Show resolved Hide resolved
vizro-core/src/vizro/figures/kpi_cards.py Outdated Show resolved Hide resolved
Copy link
Contributor

@maxschulz-COL maxschulz-COL left a comment

Choose a reason for hiding this comment

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

🚀 😍 Really love this new feature! I think it is definitely a good addition.

Provided a few comments, but nothing crucial. For the name, I wanna ponder a little more, but I think I have no better idea 😂

vizro-core/src/vizro/figures/__init__.py Outdated Show resolved Hide resolved
vizro-core/src/vizro/models/_components/figure.py Outdated Show resolved Hide resolved
vizro-core/src/vizro/figures/kpi_cards.py Outdated Show resolved Hide resolved
vizro-core/src/vizro/figures/kpi_cards.py Outdated Show resolved Hide resolved
vizro-core/src/vizro/figures/kpi_cards.py Show resolved Hide resolved
vizro-core/src/vizro/figures/kpi_cards.py Show resolved Hide resolved
vizro-core/examples/_dev/app.py Show resolved Hide resolved
@maxschulz-COL
Copy link
Contributor

One more random thought: are we sure about having the kpi cards in our API, as opposed to just as examples? Would avoid API changes, and maintentance, etc.?

My feeling is that we probably do want them: I am actually quite happy with supporting this API e.g. as compared to custom charts, where the whole alignment (or lack thereof) with the plotly.express API is more of a burden.

Just thought I'd raise this as a discussion point, as when we have hosted examples very soon, there might be a little less need for things to be native.

Another big pro here is of course that we can then configure KPI cards via yaml! But yh, just thought I'd raise this to gauge your view.

@huong-li-nguyen
Copy link
Contributor Author

huong-li-nguyen commented Jun 12, 2024

Another big pro here is that we can then configure KPI cards via yaml! But yeah, I wanted to raise this to gauge your view.

I feel strongly about including the KPI cards in our public API, more than any of the custom charts we plan to bring from our internal library. Apart from your arguments, it's worth adding because it's such a widespread component used in any business dashboard. Since customizing the go.Indicator for our design needs wasn't easy, this further emphasizes the need to add this component to our library. It will also bring much value to the OS community, as I've seen similar struggles on the forums.

At some point, we discussed whether we should only have one KPI card function and combine the two here. But there were a few reasons why we decided not to do that. First, we wanted to avoid exposing someone who wants a simple KPI card to an API with all these extra arguments that are also not intuitive. Hence, all of these long docstrings 😄 The other point is the concept of CapturedCallables. One of the reasons why we've introduced this is precisely for this purpose. I don't mind adding more CapturedCallables and maintaining their API as long as they are generic enough and provide value to the OS community.

Any customizations of the current KPI cards (e.g., layout changes, including subtitles or any of that sort) should live in the docs. I think the current implementations can cover 80% of what people might want to do, as any customization can be done quickly based on the existing implementations. What we did not include are KPI cards with sparklines or KPI groups that flexibly rearrange themselves based on screen width/height. For me, this can all live in docs. However, the code that can cover 80% of all KPI Cards should be added by us 👍

Copy link
Contributor

@maxschulz-COL maxschulz-COL left a comment

Choose a reason for hiding this comment

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

Had another pass - good stuff!

Copy link
Contributor

@antonymilne antonymilne left a comment

Choose a reason for hiding this comment

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

I was reading the docstrings to get some context before reviewing the docs and just left a few other small comments too. Really nice work on the PR description here, it's super clear and useful 🙏

vizro-core/src/vizro/models/_components/figure.py Outdated Show resolved Hide resolved
vizro-core/src/vizro/figures/kpi_cards.py Outdated Show resolved Hide resolved
vizro-core/src/vizro/figures/kpi_cards.py Outdated Show resolved Hide resolved
vizro-core/src/vizro/figures/kpi_cards.py Outdated Show resolved Hide resolved
vizro-core/src/vizro/figures/kpi_cards.py Outdated Show resolved Hide resolved
- "{value:.0%}": Formats the value as a percentage without decimal places.
- "{value:,}": Formats the value with comma as a thousands separator.

agg_func: String function name to be used for aggregating the data. Common options include
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 a great point @maxschulz-COL and I basically agree with both of you. It's actually pretty hard/impossible to find a definitive list of what strings work here in pandas - I've also wondered about this meany times before. It's nowhere in the official docs, and the system is basically yes, you just guess function names, and probably it will work 😆 The best reference I've found is this: https://stackoverflow.com/questions/65877567/passing-function-names-as-strings-to-pandas-groupby-aggregrate. So I would keep the text as is but add "For more information on possible functions, see https://stackoverflow.com/questions/65877567/passing-function-names-as-strings-to-pandas-groupby-aggregrate".

FYI it is deliberate that we only demonstrate how to do this with strings and keep the type-hint as str for exactly the reason @huong-li-nguyen says, so that things work in yaml straight away also. But I have deliberately chosen to not add in some validation that throws an exception if a callable is provided instead, because there's no reason to forbid that from working in Python and it would just make things more complicated than needed given these are meant to be simple copy and pastable functions.

@huong-li-nguyen huong-li-nguyen merged commit e1a96b4 into main Jun 18, 2024
32 of 33 checks passed
@huong-li-nguyen huong-li-nguyen deleted the feat/create_kpi_cards branch June 18, 2024 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request 🤓 Issue contains a feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants