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

[QA] Add unit tests and examples for kpi_card and kpi_card_reference #529

Merged
merged 161 commits into from
Jun 21, 2024

Conversation

huong-li-nguyen
Copy link
Contributor

@huong-li-nguyen huong-li-nguyen commented Jun 17, 2024

Description

  • Add unit tests for kpi_card and kpi_card_reference
  • Add cross-link to figure.md from card-button.md
  • Add section on kpi cards in figure.md
  • Update feature examples dashboard

Screenshot

Screenshot 2024-06-21 at 11 01 19

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 / or (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
Copy link
Contributor Author

@stichbury @antonymilne - ready for another round 👍 You guys can review again while I'll create the yaml example

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.

The docs look great! The tests need a little bit more work I think, but I trust you to do it and merge whenever you're happy with it.

Did the code for KPI card and KPI reference card change? I don't remember it having all the CardHeader stuff in it before, but it looks like a great way to do it. There's just one thing I'd suggest refactoring. Currently it looks like this:

 return dbc.Card(
        [
            dbc.CardHeader(
                [
                    html.P(icon, className="material-symbols-outlined") if icon else None,
                    html.H2(title),
                ],
            ),
            dbc.CardBody(value_format.format(value=value)),
        ],
        className="card-kpi",
    )

Let's break it up a bit like this:

    header = dbc.CardHeader([html.P(icon, className="material-symbols-outlined") if icon else None, html.H2(title)])
    body = dbc.CardBody(value_format.format(value=value))
    return dbc.Card([header, body], className="card-kpi")

And similarly for the kpi_card_reference. Normally I wouldn't care and would probably write it as you have done yourself, but given that these are meant to be easy to understand copy and pastable functions I think better to make them look less intimidating. Exactly the same logic as my suggestion for how to rewrite that long df with all the text in it.

Remember that you're actually writing code for multiple different purposes here:

  • "normal" code read by experienced developers
  • test cases read by experienced developers but should be easy to run the tests in my head
  • copy and pastable code for users who can code but don't know Dash well
  • docs read by all users

Each of these has slightly different expectations/acceptable level of complexity so the code style can change between them rather than being one size fits all. e.g. stuff with lots of nesting weirdly formatted by black is fine for me as an experienced developer but not so digestible to a user reading docs.

vizro-core/docs/pages/user-guides/custom-figures.md Outdated Show resolved Hide resolved
vizro-core/tests/unit/vizro/figures/test_kpi_cards.py Outdated Show resolved Hide resolved
vizro-core/tests/unit/vizro/figures/test_kpi_cards.py Outdated Show resolved Hide resolved
vizro-core/tests/unit/vizro/figures/test_kpi_cards.py Outdated Show resolved Hide resolved
vizro-core/tests/unit/vizro/figures/test_kpi_cards.py Outdated Show resolved Hide resolved
vizro-core/tests/unit/vizro/figures/test_kpi_cards.py Outdated Show resolved Hide resolved
vizro-core/tests/unit/vizro/figures/test_kpi_cards.py Outdated Show resolved Hide resolved
Copy link
Contributor

@stichbury stichbury left a comment

Choose a reason for hiding this comment

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

LGTM, loads of great and useful content. Thanks and well done 🙏

@huong-li-nguyen
Copy link
Contributor Author

Did the code for KPI card and KPI reference card change? I don't remember it having all the CardHeader stuff in it before, but it looks like a great way to do it. There's just one thing I'd suggest refactoring. Currently it looks like this:

Yes, you probably didn't see my comment there then haha I've changed it here: #493 (comment) after our discussion on not having so many different classNames in the functions.

Each of these has slightly different expectations/acceptable level of complexity so the code style can change between them rather than being one size fits all. e.g. stuff with lots of nesting weirdly formatted by black is fine for me as an experienced developer but not so digestible to a user reading docs.

Yeah, that makes sense - will pay more attention to that now! 👍

@antonymilne
Copy link
Contributor

Oooh yes I didn't see the dbc.CardHeader etc. stuff before but it looks like the best possible solution 👍 In general I like using these bootstrap native components - even if they're just doing html.Div with a class name it's preferable to us doing that with our own class names.

@huong-li-nguyen huong-li-nguyen enabled auto-merge (squash) June 21, 2024 09:10
@huong-li-nguyen huong-li-nguyen merged commit c998e8e into main Jun 21, 2024
33 checks passed
@huong-li-nguyen huong-li-nguyen deleted the qa/add-kpi-card branch June 21, 2024 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docs 🗒️ Issue for markdown and API documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants