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] Enable variant argument inside Container #1002

Merged
merged 52 commits into from
Mar 4, 2025

Conversation

huong-li-nguyen
Copy link
Contributor

@huong-li-nguyen huong-li-nguyen commented Feb 10, 2025

Description

  • Replace html.Div with dbc.Container in the underlying model. We enable fluid=True to ensure containers expand to 100% of the available width, following Bootstrap's container behavior: Bootstrap Containers.
  • Add support for the variant argument in vm.Container(..., variant="filled"). Available options: "filled", "outlined" and "plain" with "plain" being the default
  • Ensure background color consistency by matching it with the Card background color.
  • Update docs and tests

Note on "elevated": We need more testing on both the design and development side. Box shadows are not visible in dark mode, so to make "elevated" work, we'd have to use both shadows and a background color. But in a dark theme, that would end up looking like "filled." Plus, we'd need dynamic margin adjustments to keep shadows from getting cut off. After discussing with J., we’ve decided to put "elevated" on hold for now since it wasn’t part of the original plan.

Screenshot

Screenshot 2025-03-01 at 22 02 41 Screenshot 2025-03-01 at 22 02 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 huong-li-nguyen changed the title Dev/styled containers [FOR DISCUSSION] Dev/styled containers Feb 10, 2025
Copy link
Contributor

github-actions bot commented Feb 10, 2025

View the example dashboards of the current commit live on PyCafe ☕ 🚀

Updated on: 2025-03-04 17:28:05 UTC
Commit: 3de3b83

Compare the examples using the commit's wheel file vs the latest released version:

vizro-core/examples/scratch_dev

View with commit's wheel vs View with latest release

vizro-core/examples/dev/

View with commit's wheel vs View with latest release

vizro-core/examples/visual-vocabulary/

View with commit's wheel vs View with latest release

vizro-ai/examples/dashboard_ui/

View with commit's wheel vs View with latest release

@huong-li-nguyen huong-li-nguyen changed the title [FOR DISCUSSION] Dev/styled containers [Feat] Enable background color inside Container Feb 17, 2025
@huong-li-nguyen huong-li-nguyen marked this pull request as ready for review February 17, 2025 10:11
@huong-li-nguyen
Copy link
Contributor Author

@antonymilne and @petar-qb or @nadijagraca - could I ask you guys for a new review? We decided to not introduce the background argument but have a theme argument instead that allows the user to choose from a pre-selection of styles.

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.

[EDIT] - This comment is moved into #1002 (comment) to continue the conversation in that comment thread.


First of all, I'm so happy we abandoned the background=True/False idea! 😅

As theme feels misleading, I'll use style_variant in the rest of the comment as it feels much more natural to me 😄

Before merging this PR, I think we should establish a rough plan for style_variant. In future I see that we will upgrade this solution to another level and that we will have a predefined set like "transparent", "filled", "outlined", "elevated", "rounded"... In that case each component could have a default like for example, "transparent" for Container, "filled" for Card and Alert, while Button might include additional options like "link" and "disabled".

Open questions:
1. Should we default Container to "transparent" instead of None?
2. Should we align our style_variant values with existing standards like Bootstrap, or define our own? Caution: users could require something like "rounded-elevated" 🤔

While we don’t need a full plan before merging, having a clear direction will help ensure consistency. Looking forward to your thoughts! 🚀

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.

This is amazing, thank you for all your work on it! I know it's taken a lot of discussion 😅 It looks great and happy to approve now subject to a few things being resolved:

  • finalise argument name
  • decide what should be the possible values for theme (or whatever name it has) - something other than None might be better
  • a couple of new tests

There's two other issues worth considering but I don't think we need to do anything about them before merging.

  1. Implications this has for vm.Card and proposed new Text component - let's discuss in our 1-1 tomorrow
  2. Check out this Pycafe example (screenshot below). Do we care that there's no way to turn the background color back to the original unfilled colour in the case of a filled container inside a filled container? i.e. the top right is the same as the bottom right and there's not an easy way to get the inverse background color (how would you do it with extras={"className": ...}?). Not a big deal for me and I don't want to add lots of extra complexity or any crazy dynamic changing of class names depending on parent containers. But maybe there should be another available theme that somehow does unfilled. But then what if you want to do unfilled + border, it would need to be another option... So probably best just to ignore this I think unless you have a bright idea 😁

image

@huong-li-nguyen
Copy link
Contributor Author

huong-li-nguyen commented Mar 4, 2025

This is amazing, thank you for all your work on it! I know it's taken a lot of discussion 😅 It looks great and happy to approve now subject to a few things being resolved:

  • finalise argument name
  • decide what should be the possible values for theme (or whatever name it has) - something other than None might be better
  • a couple of new tests
  • I went with variant
  • Possible options are filled,outlined or plain - In the future we may or may not add elevated.
  • Added unit tests here: 8bab882
  1. Yes, let's discuss in our 1:1.
  2. Yes, I did briefly discuss this case with J. and we decided to ignore it as it's such an edge case. We can in practice always iterate the filled color though. So on every second iteration, we go back to the darker color. The CSS would look like this:
/*This handles your example*/
.bg-container .bg-container {
  background-color: var(--right-side-bg);
}

/*Handles further nesting*/
.bg-container .bg-container .bg-container {
  background-color: var(--surfaces-bg-card);
}

.bg-container .bg-container .bg-container .bg-container {
  background-color: var(--right-side-bg);
}

...
Screenshot 2025-03-04 at 14 46 46

If we think we want to support that, I can just add this into our static CSS, but I am also fine just not encouraging people to do that haha 😄

@huong-li-nguyen huong-li-nguyen requested a review from petar-qb March 4, 2025 13:51
@huong-li-nguyen huong-li-nguyen changed the title [Feat] Enable theme argument inside Container [Feat] Enable variant argument inside Container Mar 4, 2025
@huong-li-nguyen
Copy link
Contributor Author

If we think we want to support that, I can just add this into our static CSS, but I am also fine just not encouraging people to do that haha 😄

Just talked with J. and he wanted the first level of nesting to work, so I'll add that CSS, in which case your example will also work out of the box @antonymilne

@huong-li-nguyen huong-li-nguyen enabled auto-merge (squash) March 4, 2025 17:26
@huong-li-nguyen huong-li-nguyen merged commit de81e39 into main Mar 4, 2025
40 checks passed
@huong-li-nguyen huong-li-nguyen deleted the dev/styled-containers branch March 4, 2025 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants