Refactor Page Layout components to render correctly in Custom Views panels #3353
Conversation
…tter in custom view panels
🦋 Changeset detectedLatest commit: 82a180e The changes in this PR will be included in the next version bump. This PR includes changesets to release 38 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| @@ -140,6 +142,13 @@ function CustomViewShell(props: TCustomViewShellProps) { | |||
| [] | |||
| ); | |||
|
|
|||
| useLayoutEffect(() => { | |||
There was a problem hiding this comment.
This is the simplest solution.
I think I would prefer to update how we generate the HTML document when running/building the application to already include this attribute but I was worried that end-users would need to make sure they update all app-kit dependencies at the same time, and last year we learnt that's not always the case.
Pelase, let me know waht you think 🙏
There was a problem hiding this comment.
I would prefer to avoid using an effect for this case.
Do we need to target the body? I think it would be enough to define the data attribute in one of the wrapping containers of CustomViewShell, for example
<ContentWrapper data-extension-type="custom-view">There was a problem hiding this comment.
Thanks. That's a better approach for our current use case.
Updated here: 5694676
| @@ -19,6 +20,9 @@ const DetailPageContainer = styled.div` | |||
| background-color: ${appKitDesignTokens.backgroundColorForPageHeader}; | |||
| padding: ${appKitDesignTokens.paddingForDetailPageHeader}; | |||
| border-bottom: 1px ${appKitDesignTokens.colorForPageHeaderBottomBorder} solid; | |||
| body[data-extension-type='${CUSTOM_EXTENSION_TYPES.CUSTOM_VIEW}'] & { | |||
There was a problem hiding this comment.
Basically we overwrite the paddings or margins in all the relevant components if they are rendered within a Custom View.
|
Deploy preview for merchant-center-application-kit ready! ✅ Preview Built with commit 82a180e. |
|
Deploy preview for application-kit-custom-views ready! ✅ Preview Built with commit 82a180e. |
|
We decided to block this PR as the proposed change is not agreed by the team and we need to have a discussion between engineers and designers in order to come to an alignment. |
@emmenko and I have been discussing about the current approach and, although he's concern on the maintainability costs this implementation might have in the future, we agreed that the balance in terms of added complexity and developer experience is fair enough as we also don't see better approaches that do not imply either degrading DX or including some complexity anyway. We discussed options like:
At the end of the day, we think adding this extra complexity it worth the DX os developers not needing to do anything special in terms of what components and paddings to use no matter the Custom View panel they's using (or if they are using page layout components in Custom Applications or Custom Views). |
CarlosCortizasCT
requested review from
emmenko
and removed request for
a team
| @@ -140,6 +142,13 @@ function CustomViewShell(props: TCustomViewShellProps) { | |||
| [] | |||
| ); | |||
|
|
|||
| useLayoutEffect(() => { | |||
There was a problem hiding this comment.
I would prefer to avoid using an effect for this case.
Do we need to target the body? I think it would be enough to define the data attribute in one of the wrapping containers of CustomViewShell, for example
<ContentWrapper data-extension-type="custom-view">| // Bear in mind the margins are dependant on the context the component | ||
| // is rendered in. For example, when rendered in a custom view, its panel | ||
| // already contains default paddings, so we don't need to add margins here. |
There was a problem hiding this comment.
I would prefer to keep the comments out of the CSS, as otherwise they will be bundled with the styles
There was a problem hiding this comment.
Updated here: cf1c6e5
However, I checked the built bundles and I couldn't find those comments, although I could have missed something.
| <div | ||
| css={css` | ||
| margin: ${appKitDesignTokens.marginForModalPageHeader}; | ||
| padding: ${appKitDesignTokens.paddingForModalPageHeader}; | ||
| border-bottom: 1px solid | ||
| ${appKitDesignTokens.borderColorForModalPageHeaderDivider}; | ||
| body[data-extension-type='${CUSTOM_EXTENSION_TYPES.CUSTOM_VIEW}'] & { |
There was a problem hiding this comment.
This is a bit "surprising" to me that we can target global elements within a scoped style.
I would suggest to follow this approach (@ByronDWall actually proposed it 😅): emotion-js/emotion#2836 (comment)
css`
* :where([data-extension-type="custom-view"]) & {
margin: ${appKitDesignTokens.paddingForModalPageHeaderInCustomView};
}
`There was a problem hiding this comment.
Updated here: cf1c6e5
packages/application-components/src/components/internals/tabular-page.tsx
Outdated
Show resolved
Hide resolved
| // Bear in mind the margins are dependant on the context the component | ||
| // is rendered in. For example, when rendered in a custom view, its panel | ||
| // already contains default paddings, so we don't need to add margins here. |
There was a problem hiding this comment.
I forgot about that one. Sorry.
Updated here: 64f6bef
| body[data-extension-type='custom-view'] & { | ||
| margin: ${appKitDesignTokens.marginForCustomViewsSelectorAsTabularInCustomView}; | ||
| } |
There was a problem hiding this comment.
I guess this also needs to be adjusted
There was a problem hiding this comment.
I forgot about that one. Sorry.
Updated here: 129d8b9
| // Bear in mind the margins are dependant on the context the component | ||
| // is rendered in. For example, when rendered in a custom view, its panel | ||
| // already contains default paddings, so we don't need to add margins here. |
There was a problem hiding this comment.
I forgot about that one. Sorry.
Updated here: 64f6bef
| marginForDialogContainerContents: `${uiKitDesignTokens.spacing30} 0 ${uiKitDesignTokens.spacing50} 0`, | ||
| marginForModalPageHeader: `0 ${appKitSpacing55}`, | ||
| marginForUserMenuItem: `${uiKitDesignTokens.spacing10} 0`, | ||
| marginLeftForModalPageHeaderControls: uiKitDesignTokens.spacing50, | ||
| marginRightForAppbar: appKitSpacing55, | ||
| marginTopForDialogFooter: uiKitDesignTokens.spacing50, | ||
| marginForPageContent: `${uiKitDesignTokens.spacing50} ${appKitSpacing55}`, | ||
| marginForPageContentInCustomView: `${uiKitDesignTokens.spacing50} 0`, |
There was a problem hiding this comment.
As an alternative idea / approach, would it work if we override the css variables (theming) to change the margins instead of doing that explicitly in each component? 🤔
So basically in the CustomViewShell we inject styles in the wrapper and override the css variables.
What do you think?
There was a problem hiding this comment.
Thanks @emmenko. That's a great idea 🔝
The only drawback that I can see is that maybe it's a little more complicated to spot where things are happening in terms of maintanability.
With the new approach, the Page Layout components have a different UI when they are rendered within a Custom View Panel but it is not pretty straightforward why as the code which makes that happen is not in those components but in a new file (custom-view-shell.styles.ts).
I've opened this PR with the refactoring so we can all take a look at how it would look like before applying those changes here.
@commercetools/shield-team-fe it would be great to read your thoughts 🙏
There was a problem hiding this comment.
Thanks Carlos. I left a review in the other PR 🤗
There was a problem hiding this comment.
I merged the other PR into this one so we have your idea implemented here.
…w panels (#3378) * refactor(application-components): refactor how we manage double padding when page layout components are used within custom views panels * refactor(application-shell): remove unnecessary styles
| @@ -15,6 +15,5 @@ export const MainPageContent = styled.div` | |||
| flex: 1; | |||
| flex-basis: 0; | |||
| overflow: auto; | |||
| // NOTE: do not change to "padding" as this breaks sticky DataTable styles | |||
There was a problem hiding this comment.
Nit: could you keep and move this comment outside of the styles (L13)?
There was a problem hiding this comment.
Restored here: 82a180e
CarlosCortizasCT
deleted the
cm-page-layout-components-in-custom-views
branch
Summary
Refactor Page Layout components to render correctly in Custom Views panels
Description
When rendering this kind of component (eg:
InfoMainPage, we noticed we were introducing a "double padding" effect since the Custom Views panel itself already contains some paddings to make sure, no matter what content is rendered inside, the contents have some white space around and these kind of components also render some surrounding paddings/margins.To avoid this unwanted UI, we now add an attribute to the
bodyelement of the Custom View panel iFrame we render inside it and use it to adjust the CSS rules of those components when they are rendered in such context.BEFORE:
AFTER:
