Skip to content
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

Composer - add animated bottom border #4325

Merged
merged 9 commits into from
Jun 3, 2024
Merged

Composer - add animated bottom border #4325

merged 9 commits into from
Jun 3, 2024

Conversation

mozzius
Copy link
Member

@mozzius mozzius commented Jun 3, 2024

  • Adds an animated bottom border when content underlays it
  • Tweak icons in bottom bar
  • Reduce keyboard vertical offset since I was seeing a white strip on device (not in simulator though? still figuring it out)
Simulator.Screen.Recording.-.iPhone.15.-.2024-06-03.at.09.38.08.mp4

Copy link

render bot commented Jun 3, 2024

Copy link

github-actions bot commented Jun 3, 2024

Old size New size Diff
7.38 MB 7.38 MB -355 B (-0.00%)

@gaearon gaearon merged commit 891b432 into main Jun 3, 2024
6 checks passed
@gaearon gaearon deleted the samuel/bottom-border branch June 3, 2024 23:49
Comment on lines +689 to +696
const onScrollViewLayout = useCallback(
(evt: LayoutChangeEvent) => {
runOnUI(showHideBottomBorder)({
newScrollViewHeight: evt.nativeEvent.layout.height,
})
},
[showHideBottomBorder],
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In these cases, the preferred method seems to be to just update the SharedValue from this callback, and make hasScrolledBottom a derived value with useDerivedValue. Makes it a little easier and lets us avoid the showHideBottomBorder function all together.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw i remember bumping into issues with useDerivedValue in the past and having to rewrite those but i don’t remember why. so just worth checking that it works well if we switch it

Copy link
Contributor

@haileyok haileyok Jun 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooo, yea do check then. it shouldn't necessarily matter since either from the JS thread is async so if this is more reliable it seems fine, unsure if the suggestion in their docs is just for code cleanliness or if there's some other reason as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see a way to derive a value from multiple inputs?

Copy link
Contributor

@haileyok haileyok Jun 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should just be able to use it like so, and the returned value will update once one or multiple of the shared values changes

const hasScrolledBottom = useDerivedValue(() => {
  return withTiming(
    contentHeight.value - contentOffset.value > scrollViewHeight.value ? 1 : 0,
  )
})

@haileyok
Copy link
Contributor

haileyok commented Jun 4, 2024

Ahh! The height in onContentSizeChange and layout.height in onScrollViewLayout are given in 3rds, whereas the contentOffset.y is a whole number 🤯

I think if we floor the height and layout.height then just check for contentHeight.value - contentOffset.value > svHeight.value we'll get the expected result! Would explain why it works on and off since it's probably +/- 1 at every position.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants