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

Panel component #1926

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

giovanniTramonto
Copy link
Contributor

Here is the proposal for panel-sections that I mentioned here #1920

I placed it in /web-componets to not conflict with /views, but could be moved later if preferred, of course.
Its a custom element instead of shadow. CSS inheritance persists.
I adjusted addStyleSheet in utils so using custom-elements multiple times loads the style only once.
Actually wondering if the use of private properties in classes meets the project conventions, but found it useful to isolate local styles from extends.

I applied it only to DesignSpace-Navigation for now for this proposal. But using it all across the sidebars will improve consistency and make a lot of style repetitions redundant.

@justvanrossum
Copy link
Collaborator

I will need a bit of time to digest this.

Private properties is indeed something we don't yet use (mostly because I didn't know about them at the start of the project), but I totally see how they are useful, even if the syntax is a bit jarring if you aren't used to it.

I'm not sure about that change in addStyleSheet: you check the document for presence of the style sheet, but add it to the element if it's not in the document. This feels a bit weird to me.

@giovanniTramonto
Copy link
Contributor Author

giovanniTramonto commented Jan 9, 2025

Yes, it’s a bit crude. Is there another approach in the project for appending components multiple times?
I mean actually, the .panel css module is just a few lines, it could simply go to editor.css. Of course, ideally styles and template are in the same place, but that would add a lot of complexity, I guess.
Perhaps better to bury the approach in this MR another time and to simply add some editor css classes that can be used …

@giovanniTramonto
Copy link
Contributor Author

I tried this out giovanniTramonto@2fc0fe3
Styles got simpler, template bigger 😄
But more consistent.

@justvanrossum
Copy link
Collaborator

But now the "template for everything contains" styles for something specific, that may not be needed for everything that includes "shared.css". So that feels a bit off to me.

@giovanniTramonto
Copy link
Contributor Author

Yes, would be a work around :/
Actually, utility classes could be really helpful in this scenario. I’ll give it a try later.

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.

2 participants