Closed huong-li-nguyen closed 1 week ago
I'm not sure why the linkchecker in the RTD build is upset -- it could just be that it's expecting a page to exist that won't exist until after this PR merges?
I'm not sure why the linkchecker in the RTD build is upset -- it could just be that it's expecting a page to exist that won't exist until after this PR merges?
Yes, I think so too 👍 I'll create the PR to fill that docs page soon and then I'll just merge these subsequently 👍
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.
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 👍
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
, andAgGrid
from theFigure
, see Antony's comment here: https://github.com/mckinsey/vizro/pull/493#pullrequestreview-2100342271As 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: https://github.com/mckinsey/vizro/pull/493#discussion_r1627671410I'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: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.1) Use a custom chart with go.Indicator and provide to vm.Graph ❌
2) Use a custom component ❌
3) Extend the vm.Card model with a figure attribute ❌
text
andhref
. While it was possible, there were several drawbacks to this approach:dbc.Card
outer container from the CapturedCallable, which led to several issues given classNames needing to be provided4) Create a new component with a figure attribute ✅
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:
Notice
[x] I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":