-
Notifications
You must be signed in to change notification settings - Fork 34
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
fix: get app content out of shadow DOM altogether #2488
base: main
Are you sure you want to change the base?
fix: get app content out of shadow DOM altogether #2488
Conversation
@@ -166,6 +132,28 @@ export type UiShellContentProps = { | |||
}; | |||
} & UiShellNavComponentProps; | |||
|
|||
const appWindowStyles = { | |||
border: "1px solid red", |
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.
Was this meant to be in 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.
doh, test code, will remove
elementToStyle.style.setProperty("position", "absolute"); | ||
elementToStyle.style.setProperty("top", `${boundingRect.y}px`); | ||
elementToStyle.style.setProperty("left", `${boundingRect.x}px`); | ||
elementToStyle.style.setProperty("width", `${boundingRect.width}px`); | ||
elementToStyle.style.setProperty("height", `${boundingRect.height}px`); |
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 may be missing something real obvious, but why are we inlining these styles on the element instead of using className or emotion or something?
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.
We're handed a raw HTMLDivElement, so there's not really a good way to do a generated class or emotion or whatnot. Additionally the values are computed, so it would be constantly changing. This is the "good enough" solution IMO.
[hasStandardAppContentPadding, odysseyDesignTokens], | ||
); | ||
|
||
useEffect(() => { |
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.
Do we need to debounce this? Is this going to run everytime the browser invokes the animate state during a resize in progress?
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.
Good thought. That (debounce) is the purpose of the requestAnimationFrame / cancelAnimationFrame below on L205-206
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.
Thanks. I read more about this after seeing your reply. Interesting that requestAnimationFrame
handles this in a performant way.
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.
Changes made over Zoom. Approved!
a90f80d
REPLACE_WITH_JIRA_TICKET_NUMBER
Summary
Testing & Screenshots