-
Notifications
You must be signed in to change notification settings - Fork 290
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
Add class property to box component #990
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I think I like this because I've been running into some issues with our style limitations, particularly around hover.
We may need to eventually namespace our CSS class names though if we run into stylesheet conflicts (it's already been possible but now that we're encouraging 3rd party sylesheets with this feature, it may become more prevalent), particularly with our CSS class names in:
mesop/mesop/web/src/app/styles.scss
Line 4 in bb8fd98
@use 'external/npm/node_modules/@angular/material/index' as mat; |
Also, Angular Material introduces a lot of CSS classnames but they are pretty disciplined about namespacing it with mat-
or mdc-
so I don't think it's too problematic unless someone is trying to inject a material stylesheet (which would be quite surprising and probably isn't something we need to worry about).
Since this PR looks like it depends on the previous PR around static file serving, I'll hold off on LGTM-ing (alternatively, you could load the CSS files from a CDN)
@@ -14,13 +14,15 @@ | |||
def box( | |||
*, | |||
style: Style | None = None, | |||
classes: str = "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like below you're supporting both "foo bar"
and ["foo", "bar"]
? I would type this as list[str] | str
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's what I wanted to support initially. But ran into an issue with that annotation. I think just needed to update the code in the component maker code to handle that annotation. But since I just wanted a to get a quick draft up, just left it as is.
But I guess, I wonder if we need to allow a list. We could have a me.convert_class_list type helper if people are dynamically generating class names. I think I'd lean toward allowing people to pass in lists for the dynamic class name case rather than introducing a helper function.
@@ -1,4 +1,4 @@ | |||
<mat-sidenav-container class="container"> | |||
<mat-sidenav-container class="sidenav-container"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious, was this causing a naming conflict with bootstrap?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, they have a class named "container". So when I first tried loading the bootstrap.css, the layout was messed up and eventually tracked it down to this usage.
Re: tailwind, I think we could try using the |
Good point. I assumed it wouldn't work, but I never actually tried it. I'll give it try when I get the chance. |
Yeah, agree about prefixing
Yeah, I figured this feature would be more useful/user-friendly if we could serve static files directly from Mesop, otherwise it makes the barrier to usage a bit hard (either upload custom styles to a CDN or mount the FastAPI server). This is mainly for the tailwind case where we need to generate a custom css file. And also the custom css stylesheet in general case. With bootstrap, can just pull from a CDN. |
Going to close this one out. Created a version without the dependency on the static files pull request. New pull request here: #995 |
One thing that I think can be help is being able to expose CSS classes so that people can leverage existing layout frameworks. This doesn't really help people that are new to frontend. But could help people that are familiar with these frameworks.
Bootstrap example
Just copied the Tailwind example (but still need to test out more layouts
Tailwind set up
For Tailwind there's a way to manually specify the CSS names you want, so we can potential read the page file:
Then you can add it to the tailwind.config.js to generate the specific CSS