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
chore: address new works for you comments #9935
base: main
Are you sure you want to change the base?
Conversation
c7c32b9
to
5506987
Compare
5506987
to
3d224d8
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.
Thanks for the fixes! I've added some ideas about simplifying the hacky part :)
const [hasChangedLayout, setHasChangedLayout] = useState(false) | ||
|
||
const firstUpdate = useRef(true) | ||
|
||
// This is a hack to force the grid to re-render when the layout changes | ||
// Flashlist makes a werid animation when changing between numColumns | ||
// We avoid it by showing the placeholder for a second while it's happening | ||
useLayoutEffect(() => { | ||
if (firstUpdate.current) { | ||
firstUpdate.current = false | ||
return | ||
} | ||
|
||
setHasChangedLayout(true) | ||
}, [numColumns]) | ||
|
||
if (hasChangedLayout) { | ||
setTimeout(() => { | ||
setHasChangedLayout(false) | ||
LayoutAnimation.configureNext(LayoutAnimation.Presets.easeInEaseOut) | ||
}, 1000) | ||
|
||
return <NewWorksForYouPlaceholder /> | ||
} |
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 do you think about moving this logic into MasonryInfiniteScrollArtworkGrid
and converting it into a hook to avoid duplication? I guess, this issue will arise wherever i't possible to change the number of columns.
const artworks = extractNodes(data.artworks) | ||
const numColumns = defaultViewOption === "grid" ? NUM_COLUMNS_MASONRY : 1 | ||
|
||
const [hasChangedLayout, setHasChangedLayout] = useState(false) |
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 using/overriding a layout animation here, could we simply show the placeholder between the column change? I'm thinking of something like this inside of MasonryInfiniteScrollArtworkGrid
:
const [internalNumColumns, setInternalNumColumns] = useState(numColumns)
const [showPlaceholder, setShowPlaceholder] = useState(false)
useEffect(() => {
showPlaceholder(true)
}, [numColumns])
useEffect(() => {
if (!showPlaceholder) return
setInternalNumColumns(numColumns)
showPlaceholder(false)
}, [showPlaceholder])
useEffect(() => { | ||
if (hideTitle) { | ||
if (currentScrollY >= NAVBAR_HEIGHT) { | ||
setHideTitle(false) | ||
} | ||
} | ||
}, [currentScrollY]) |
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 PR resolves ONYX-840
Description
The goal from this PR is addressing the design comments on the New Works For You screen raised by Barney and removing the old logic.
Before
Screen.Recording.2024-03-06.at.12.11.59.mov
After
Screen.Recording.2024-03-12.at.17.04.53.mov
PR Checklist
To the reviewers 👀
Changelog updates
Changelog updates
Cross-platform user-facing changes
iOS user-facing changes
Android user-facing changes
Dev changes
Need help with something? Have a look at our docs, or get in touch with us.