-
Notifications
You must be signed in to change notification settings - Fork 36
DOP-6049 Introduces the MultiColumn Component #1504
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
Conversation
✅ Deploy Preview for docs-frontend-stg ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
❌ Deploy Preview for docs-frontend-stg failed
Error Details
|
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.
Some small comments but the staging link looks very nice!
"children": [ | ||
{ | ||
"type": "text", | ||
"value": "This is a paragraph between the code blocks." |
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.
Is this like the final text? Or like are giving this version to the writers but they will be filling out the content
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.
@biancalaube this is just test data. The content comes from the writers in the src of the landing directory from the monorepo.
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.
see comments below! also wondering if this is safe to release without the content change (would be good to see a regression check!)
src/components/MultiColumn/index.tsx
Outdated
margin-top: 60px; | ||
margin-bottom: 40px; | ||
@media ${theme.screenSize.upTo2XLarge} { |
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.
should this be lining up with tablet view? seems like it switches to vertical layout too early
&::before { | ||
content: url('data:image/svg+xml,<svg xmlns="http://www.w3.org/2000/svg" width="10" height="10" viewBox="0 0 10 10" fill="none"><circle cx="5" cy="5" r="4.25" stroke="%2300ED64" stroke-width="1.5"/></svg>'); | ||
position: absolute; | ||
left: 8px; |
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.
is this left
intentional? seems a little closer than the figma design
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.
The left
is internal, but I needed to give it more space from the text and had to increase the spacing.
The content will be added by the writers, but this allows them to be able to use the content. I don't think it will go pass preprd. |
Also, we did add a snapshot test as well. |
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.
minor comments below, but overall looks good 👍
src/components/Code/Code.tsx
Outdated
removeMargin, | ||
}: { | ||
nodeData: CodeNode; | ||
removeMargin: boolean; |
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.
instead of removeMargin prop, can we take className and append it to the class, and style in the consuming component
src/components/MultiColumn/index.tsx
Outdated
import { theme } from '../../theme/docsTheme'; | ||
|
||
const MultiColumn = ({ nodeData }: { nodeData: ParentNode }) => { | ||
console.log('nodeData', nodeData); |
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.
minor, remove log
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 callout, I forgot to remove this.
✅ Deploy Preview for landing ready
|
Stories/Links:
DOP-6049
Current Behavior:
Put a link to current staging or production behavior, if applicable
Staging Links:
https://deploy-preview-13580--mongodb-landing.netlify.app/docs/
Notes:
This PR introduces the MultiColumn Component. This component will be used on the homepage. You can test locally by also pointing to the PR branch in the snooty-parser
side note: Waiting to also hear back from the design team on look and feel for mobile/tablet view
README updates