Skip to content

Conversation

nitzanyiz
Copy link
Collaborator

@nitzanyiz nitzanyiz commented Dec 6, 2023

Description

There were labels near the values of the wheel picker. Because they were white they could only be seen on darker backgrounds. I added opacity = 0 because removing this label was causing the entire layout to change.

Changelog

WheelPicker - fixed value labels showing near every label.

Additional info

WOAUILIB-3875

… white and the background is also white. If you put any darker color you were seeing it
@nitzanyiz nitzanyiz requested a review from M-i-k-e-l December 6, 2023 11:03
@nitzanyiz nitzanyiz marked this pull request as ready for review December 6, 2023 14:50
@@ -222,7 +222,7 @@ const WheelPicker = ({
const fakeLabelProps = useMemo(() => {
return {...labelMargins, ...labelProps};
}, [labelMargins, labelProps]);

const itemLabelStyle = useMemo(() => ({...labelStyle, opacity: 0}), [labelStyle]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd put it in the item itself.
I'd probably write it as [labelStyle, {opacity}] but now I've seen this article:

  1. Best Practices for Styling Performance
    ...
    Flattening Styles: Using arrays for styles can be computationally expensive. Instead, use StyleSheet.flatten to merge style objects.

So maybe more investigation is needed.

@nitzanyiz nitzanyiz requested a review from M-i-k-e-l December 26, 2023 11:39
@nitzanyiz
Copy link
Collaborator Author

I moved the entire opacity add to the fake label in the Item component. I used composed instead of flatten. Because its better if the fakeLabelStyle is not passed and because we don't know if the labelStyle is a stylesheet object.

@M-i-k-e-l
Copy link
Collaborator

I moved the entire opacity add to the fake label in the Item component. I used composed instead of flatten. Because its better if the fakeLabelStyle is not passed and because we don't know if the labelStyle is a stylesheet object.

Why does it matter?

BTW, compose doesn't really do anything really (link) while flatten does (link)

@nitzanyiz
Copy link
Collaborator Author

Compose returns an array if both are present and only the object if only one is preset. So if only one given then it never creates an array. I didn't see much difference between them other than that

@M-i-k-e-l
Copy link
Collaborator

Compose returns an array if both are present and only the object if only one is preset. So if only one given then it never creates an array. I didn't see much difference between them other than that

See the link in this comment for the reason memoized flatten can be better, and as I stated I am not sure about it and you can search further if this is indeed recommend.

@nitzanyiz
Copy link
Collaborator Author

nitzanyiz commented Dec 31, 2023

I don' really find any major difference between them. Its says that flatten uses something with the id in stylesheet but in some places theres mentions that stylesheet doesn't even use those ids anymore. I'll change to flatten any way maybe they'll use it in the future.

@M-i-k-e-l
Copy link
Collaborator

I don' really find any major difference between them. Its says that flatten uses something with the id in stylesheet but in some places theres mentions that stylesheet doesn't even use those ids anymore. I'll change to flatten any way maybe they'll use it in the future.

I saw somewhere that the docs are not updated to reflect the changed code.
You can see the code is different (I linked it above)

@M-i-k-e-l M-i-k-e-l merged commit bb7483c into master Jan 2, 2024
@nitzanyiz nitzanyiz deleted the fix/WheelPickerPhantomLabels branch January 2, 2024 13:11
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.

2 participants