✨ DOP-4791 introduces new next/prev button #1156
Conversation
There was a problem hiding this comment.
it looks really good! i'm going to leave approval for when other members of the team have gotten a chance to review things + discuss conditional class names.
one thing i noticed is the hover on light mode seems a litttle bit low contrast? i don't see anything on the figma, so maybe something to check with design?
| import { theme } from '../theme/docsTheme'; | ||
| import { getPageTitle } from '../utils/get-page-title'; | ||
| import Link from './Link'; | ||
|
|
||
| const StyledContainer = styled.div` | ||
| const styledContainer = css` | ||
| padding-top: 2em; | ||
| padding-bottom: 2.5em; |
There was a problem hiding this comment.
i noticed in the figma that we want the space between the bottom of the content and the top of the next/prev button to be 88px, although I'm not sure how strict that is
There was a problem hiding this comment.
@mayaraman19 good question, let me get a confirmation from the design team.
There was a problem hiding this comment.
@mayaraman19 the PR has been updated.
|
@caesarbell I think now it might have too much space? since the margin is 88px and there is still padding as well 
do you want to try removing the padding-top maybe? will defer to you 👍 although, i'm not sure if the spacing should also include the content's margin/padding. |
I have confirmation from the design team to implement the stacking for mobile. I will update this PR to reflect that. |
|
@mayaraman19 @rayangler , I removed the 
|
|
The spacing should be correct for mobile as well |
There was a problem hiding this comment.
This looks so great. And your styling is really clear.
There is one odd piece of the design that has made the spacing slightly different. In the mobile column layout, they have oddly given us the space from the bottom button to the TEXT of the upper button... That doesn't jive with normal block css. So, inherently our 64px rowgap is actually a bigger space than theirs.
I just inspected it further and the space between the two buttons (rather than using the text) is 51px. Could we use that or ask design if that's the correct amount?
Attaching an awful photo. I couldn't take a screenshot while holding down option lol
src/components/InternalPageNav.js
Outdated
| const titleSpanStyling = css` | ||
| line-height: 28px; | ||
| --hover-text-decoration-color: var(--underline-color) !important; | ||
| const commonNextPrevTextStyling = css` |
There was a problem hiding this comment.
total nit: Can we name this without common because it makes me think it's reused somewhere else?
There was a problem hiding this comment.
@mmeigs, thank you for your feedback. I renamed it to be a less confusing.
src/components/InternalPageNav.js
Outdated
| import { theme } from '../theme/docsTheme'; | ||
| import { getPageTitle } from '../utils/get-page-title'; | ||
| import Link from './Link'; | ||
|
|
||
| const StyledContainer = styled.div` | ||
| padding-top: 2em; | ||
| const styledContainer = css` |
There was a problem hiding this comment.
Tiny nit: Can we name it differently because it's not actually using styled anymore? I've seen patterns where using css would be named ___styles or ___styling or ___style.
There was a problem hiding this comment.
@mmeigs, I opt for __styling to keep consistent with the other styles within this component.
I can check with the design team, but it is using LG hover styles + dark mode. |
Stories/Links:
DOP-4791
Current Behavior:
MongoDB Atlas
Staging Links:
Staged: MongoDB Atlas
Staged MongoDB Manual
Notes:
Figma
Discussion with the design team
README updates