-
Notifications
You must be signed in to change notification settings - Fork 30
Add footer to gallery layout #14145
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
base: main
Are you sure you want to change the base?
Add footer to gallery layout #14145
Conversation
Hello 👋! When you're ready to run Chromatic, please apply the You will need to reapply the label each time you want to run Chromatic. |
Size Change: 0 B Total Size: 1.05 MB ℹ️ View Unchanged
|
324b92e
to
549566f
Compare
549566f
to
c9004b1
Compare
<Section | ||
fullWidth={true} | ||
data-print-layout="hide" | ||
backgroundColour={'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.
backgroundColour={'red'} | |
backgroundColour={themePalette('--apps-footer-background')} |
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.
Done
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.
✨
Approved assuming other comments are addressed
6b24fad
to
121a6e0
Compare
121a6e0
to
9988333
Compare
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 sorry I haven't had a chance to review everything yet! Some comments/questions.
} | ||
`; | ||
|
||
const galleryLeftCollumn = css` |
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 might be better called galleryBorder
? If I've understood correctly, this is sometimes in the left column and sometimes in the right one?
`; | ||
|
||
const galleryLeftCollumn = css` | ||
${grid.column.centre} |
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 these styles have a noticeable effect?
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 catch, this is un-necessary 👍
${between.tablet.and.desktop} { | ||
padding-left: ${space[5]}px; | ||
padding-right: ${space[5]}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.
Do these padding styles have a noticeable effect?
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 spot, trhis was un-necessary, removed it
${from.leftCol} { | ||
${grid.column.left} | ||
|
||
position: relative; /* allows the ::before to be positioned relative to this */ |
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.
Does the position: relative
need to be repeated in these two media queries? Can it be applied once outside them?
@@ -130,6 +140,55 @@ const syndicationButtonOverrides = css` | |||
const galleryStyles = css` | |||
${grid.paddedContainer}; | |||
background-color: ${palette('--article-inner-background')}; | |||
border-bottom: 1px solid var(--article-border); | |||
padding: 0; | |||
position: relative; |
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.
What is the position: relative
used for 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.
You're right, it's in the inner div so not needed here 👍
import { Standfirst } from '../components/Standfirst'; | ||
import { SubMeta } from '../components/SubMeta'; | ||
import { grid } from '../grid'; | ||
import type { ArticleFormat } from '../lib/articleFormat'; | ||
import type { NavType } from '../model/extract-nav'; | ||
import { palette } from '../palette'; | ||
import { palette, palette as themePalette } from '../palette'; |
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.
You probably don't need to import this twice?
import { palette, palette as themePalette } from '../palette'; | |
import { palette } from '../palette'; |
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.
Did this change in the last commit d48ed9d
What does this change?
This PR is part of the Gallery migration from frontend/AR into DCAR. It adds support for the gallery body image.
The change adds Footer to the Gallery layout for both Web & App and makes it work for both light & dark mode