-
Notifications
You must be signed in to change notification settings - Fork 22
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
Move Panel.scss to CSS modules #2745
Conversation
import paneStyles from "@/devTools/editor/panes/Pane.module.scss"; | ||
import styles from "./GenericInsertPane.module.scss"; |
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.
This is what I mentioned on Slack. I think it's the most correct solution, other than having an extendable Pane.tsx
component — which wouldn't be too bad:
// GenericInsertPane.module.scss
const GenericInsertPane => () => {
return (
<Pane title={`Build new ${config.label} extension`}>
// The actual content here
</Pane>
)
}
I could add this in this PR
f711261
to
7cab9ae
Compare
Based on the refactoring conversation yesterday, we want to move toward a common presentational/layout React components to use in the "design language" This is a good candidate for that vs. applying a module across different react components Adding @BLoe and @mnholtz since this is a good "test case" for the approach |
It was easy indeed. How is it looking? |
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.
In general, why does each one of these components need to know what a Pane
is or care that they are one, or are using one? I think "Pane" is an aspect of the Editor
layout, it's not related to any of the content or layout of each pane itself. To me, it feels like Pane should just be one wrapping <div>
with styling that we use it the Editor
to wrap the contents of each pane. This was my approach for the EditorTabLayout
I created in #2590.
What do you think about this approach? Happy to discuss it more generally here on this PR or in Slack or whatever.
& > * { | ||
height: auto !important; | ||
} |
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.
Why is this necessary?
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.
Each separate panel (which are a number of separate components) all specify height: 100%
, which does the opposite of what's expected when in a flex-direction: row
parent. You can try to remove this line to see it, then remove height: 100%
from each Editor column.
This line undoes it, as a temporary part of a long-term strategy to simplify the layout and have it behave more like an app than a web page:
The general idea is that a component should not try to set its own size (specifically "100%"); it's the parent (i.e. the local layout manager) that gives a set amount of space (or all of it) to its component.
This kind of logic would also let us have an editor with resizable columns, for example, because it's the parent that controls the width of each column.
<Nav.Item className="d-flex align-items-stretch"> | ||
<Nav.Link eventKey={step.step} className="d-flex align-items-center"> |
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.
Why bootstrap classes here?
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 just moved the style we have in Panel.scss without having to add custom CSS/classes:
pixiebrix-extension/src/devTools/Panel.scss
Lines 55 to 61 in d63eea5
.nav-item { | |
display: flex; | |
align-items: stretch; | |
} | |
.nav-link { | |
display: flex; | |
align-items: center; |
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.
Since there's already a CSS module set up for ElementWizard
, can we move these into there?
Because they contain the title of the pane and it's supposed to have the same style every where. That's basically the only reason to do this at this point. I suppose that the alternative is to move the title out of each pane and use something like: <div className={style.title}>My Blueprints</div>
<BlueprintsPane/> But some of these panes have a title that varies based on the content of the pane itself, so this isn't always feasible. Either way this React/component separation of concerns is not a question for me, 😃 I'm just following y'all’s suggestions. |
I checked out this branch and built the extension, and it looks like the entire dev tools panel scrolls vertically now, instead of just the center config area. |
I must have broken something in the last commit |
b944ec1
to
5a263e9
Compare
5a263e9
to
b527715
Compare
I reverted this PR to the first commit and ensure that the scroll areas behave as expected: Screen.Recording.movOther changes and discussions towards a React Component-based approach can be made separately as this PR is needed to fix a layout issue with the ErroBanner. |
Getting this merged in to fix the error banner, since it's impacting some ergonomics on development related to the background page General feedback
|
Something to be aware of for the new layout logic: You might see a new horizontal scrollbar in tiny windows. This is not a bug, it just replaces the old Before #2601After this PR |
Follows the suggestion in:
Fixes: