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

fix: headphones carousel example #754

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

CarlosSLoureiro
Copy link

@CarlosSLoureiro CarlosSLoureiro commented Aug 4, 2023

Summary

Fix de logic of background color from circle component at the headphones carousel example.

vp-carousel

Motivation

I've reading the code of the headphones carousel example and I understood that the color of the circle must change due to a code line will be mentioned below in this PR.

Test Plan

Just try to execute the mentioned example.

Checklist

  • I have tested this on a device and a simulator (yes, in Android and iOS)
  • I added the documentation in README.md (yes, updated the readme gif example)
  • I updated the typed files (TS and Flow) (No)

- react-native (0.71.8 -> 0.72.3)
- react-native-reanimated (2.17.0 -> 3.3.0)
- react-native-gradle-plugin (0.71.19 - added to android build)
style={[
styles.circle,
{
backgroundColor: color,
Copy link
Author

Choose a reason for hiding this comment

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

This it is the mentioned line part above. In addition to the component rendering according to the data length, the color swapping logic does not work correctly.

@@ -590,6 +590,7 @@
);
MTL_ENABLE_DEBUG_INFO = YES;
ONLY_ACTIVE_ARCH = YES;
OTHER_CFLAGS = "$(inherited)";
Copy link
Member

Choose a reason for hiding this comment

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

[high]: could you revert this change

@@ -654,6 +655,7 @@
"\"$(inherited)\"",
);
MTL_ENABLE_DEBUG_INFO = NO;
OTHER_CFLAGS = "$(inherited)";
Copy link
Member

Choose a reason for hiding this comment

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

[high]: could you revert this change

@troZee
Copy link
Member

troZee commented Aug 23, 2023

@CarlosSLoureiro could you resolve conflicts 🙏

Moreover, the iOS pipeline failed. Could you check that?

@CarlosSLoureiro
Copy link
Author

@troZee I think it's done.

@@ -14,10 +14,10 @@
"@react-navigation/native-stack": "~6.9.12",
"@react-navigation/stack": "~6.3.16",
"react": "18.2.0",
"react-native": "0.71.8",
"react-native": "0.72.3",
Copy link
Member

Choose a reason for hiding this comment

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

[high]: Could you revert that file?

Copy link
Author

Choose a reason for hiding this comment

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

When I tried to run the interpolate in the background Color it doesn't worked in 0.71.8 version. That's why I've upgraded the react native version. Maybe it's a specific issue from the current used version in the example...

@@ -2,17 +2,16 @@ PODS:
- boost (1.76.0)
- CocoaAsyncSocket (7.6.5)
- DoubleConversion (1.1.6)
- FBLazyVector (0.71.8)
Copy link
Member

Choose a reason for hiding this comment

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

[high]: Could you revert that file?

@troZee
Copy link
Member

troZee commented Aug 29, 2023

LGTM. Please address those 2 comments:
#754 (comment)
#754 (comment)

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