-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Woo Installer: Landing page redesign re-deploy #58179
Conversation
- Adds @automattic/viewport-react to the MasonryWave component to conditionally render different layouts on mobile and web. - Fixes missing key warnings on the MasonryWave component.
Link to Calypso live: https://calypso.live?image=registry.a8c.com/calypso/app:commit-ada2ecd94d2df7601a63ea07b1434d51e70631a4 |
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.
Tested again and LGTM.
Screen.Recording.2021-11-17.at.2.20.42.PM.mov
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/6979606 Thank you @allilevine for including a screenshot in the description! This is really helpful for our translators. |
Translation for this Pull Request has now been finished. |
* Add WoopLandingPage component * Add cta header component * Remove extra comment * Style landing page * Hide/Show Navigation Button when scrolling past the CTA Button * Re-organize components and use Types * Add SetupNotices to the new landing page * Add some responsive CSS and remove mixin * Remove style import and use rem * Add WoopLandingPage component * Add cta header component * Remove extra comment * Style landing page * Adds WaveMasonry component to the woop landing page * Rendering only images * changes cta-header to 1 column layout on mobile * Update client/components/masonry-wave/index.jsx * Clean up merge conflicts * Clean up landing-page prop * Clean up spacing, scroll * Add WooCommerce title * cta-header uses css-grid 1 column layout and centers content on mobile * adds @automattic/viewport-react and fixes key related warnings - Adds @automattic/viewport-react to the MasonryWave component to conditionally render different layouts on mobile and web. - Fixes missing key warnings on the MasonryWave component. * renaming cta images to a more descriptive name | removing unused cta images Co-authored-by: Rolando Perez <me@rolilink.com> Co-authored-by: Louis Laugesen <louis.laugesen@gmail.com>
ctaRef?: React.RefObject< HTMLButtonElement >; | ||
} | ||
|
||
const CtaSection: React.FunctionComponent< Props > = ( props ) => { |
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.
Sorry for being super late here 😅... Is this component still used? Should we remove it if it's not?
If it's still used, can we addd readme
, example
, and also integrate in devdocs https://wpcalypso.wordpress.com/devdocs/?
); | ||
}; | ||
|
||
const MasonryWave = ( props ) => { |
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.
Sorry for being super late here 😅... Is this component still used? Should we remove it if it's not?
If it's still used, can we addd readme
, example
, and integrate it in devdocs https://wpcalypso.wordpress.com/devdocs/? Also, I feel that the component name MasonryWave
doesn't really help in understanding what this component is used for. Is it for rendering an image grid?
useEffect( () => { | ||
if ( ! contentRef ) { | ||
return; | ||
} | ||
|
||
const handleScroll = () => { | ||
const headerHeight = headerRef?.current?.getBoundingClientRect().height; | ||
const offset = | ||
contentRef.current && headerHeight ? contentRef.current.offsetTop - headerHeight : 0; | ||
const scrollPosition = window.scrollY; | ||
const actionElement = actionsRef?.current; | ||
|
||
if ( ! actionElement ) { | ||
return; | ||
} | ||
|
||
if ( offset > 0 && scrollPosition < offset ) { | ||
actionElement.style.visibility = 'hidden'; | ||
} else { | ||
actionElement.style.visibility = 'visible'; | ||
} | ||
}; | ||
|
||
handleScroll(); | ||
|
||
window.addEventListener( 'scroll', handleScroll ); | ||
return () => { | ||
window.removeEventListener( 'scroll', handleScroll ); | ||
}; | ||
}, [ contentRef ] ); |
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.
After looking at https://github.com/Automattic/wp-calypso/pull/62080/files, I found at about this code. Can we add some comments about what this useEffect
does for posterity? Also, I noticed a new prop which should be added in the readme
wp-calypso/client/components/fixed-navigation-header/README.md
Lines 25 to 29 in 831b4c5
- `navigationItems` (`{ label: string; href: string }[]`) - The Navigations items to be shown | |
- `id` (`string`) - ID for the header (optional) | |
- `className` (`string`) - A class name for the wrapped component (optional) | |
- `children` (`nodes`) – Any children elements which are being rendered to the far right (optional) | |
- `compactBreadcrumb` (`boolean`) - Displays only the previous item URL reading "Back" in the breadcrumb (optional) |
Moreover, it seems to me that it could have been the parent component responsibility to render or not the actionElement
(perhaps by using a similar useEffect
rather than having to pass the contentRef
. Is there a reason for choosing to place this functionality 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.
@cpapazoglou Would you mind opening issues for these so we can triage them, rather than commenting on a merged PR?
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.
Sure, I just thought we can discuss it first. Added here #62166
Changes proposed in this Pull Request
CtaSection
component.contentRef
prop and show/hide the right content depending on whether it's also visible on the page.Testing instructions
Before
![](https://user-images.githubusercontent.com/811776/141930667-a17afe15-1848-43b8-ba8f-5f2750f69922.png)
After
![](https://user-images.githubusercontent.com/811776/141930655-a08ad8c3-d858-4204-b5be-040b55177021.png)
Related to #57419