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

Add carousel component #18

Merged
merged 3 commits into from
Jan 21, 2022
Merged

Conversation

justiniruuza
Copy link
Contributor

@justiniruuza justiniruuza commented Jan 20, 2022

#Note: Please do not merge as this time. I'm working with jitera-frontend from local to make sure it work before publish

Create carousel component.

Ios Demo:
https://www.loom.com/share/13533b76cf8149029f59aa9c7a7342da

@justiniruuza justiniruuza requested review from jefrydco, zrg-team, nhat-ndm1193 and sieuhuflit and removed request for jefrydco January 20, 2022 05:39
"lint": "eslint . --ext .js,.jsx,.ts,.tsx"
"lint": "eslint . --ext .js,.jsx,.ts,.tsx",
"add-library": "yarn remove @jitera/jitera-rn-ui-library && yarn cache clean && yarn add ../",
"reset": "watchman watch-del '/Users/justinphan/www/jitera-rn-ui-library/example' ; watchman watch-project '/Users/justinphan/www/jitera-rn-ui-library/example'"
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can simplify the command into:

Suggested change
"reset": "watchman watch-del '/Users/justinphan/www/jitera-rn-ui-library/example' ; watchman watch-project '/Users/justinphan/www/jitera-rn-ui-library/example'"
"reset": "watchman watch-del-all'"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jefrydco : updated

@jefrydco
Copy link
Collaborator

Anyway, In order for GitHub to able to render the preview, the format of the video should be mp4.


return (
<View ref={ref} style={style} onLayout={handleLayout}>
<FlatList
Copy link
Collaborator

Choose a reason for hiding this comment

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

it ok to use the library here, cause it has animation and something useful.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@zrg-team You mean we can use external library rather than implementing by ourself?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes you can use external library

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zrg-team: any suggestion library?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

anyway, we can choose another lib, i think there are a lot library out there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay if it stable then let go with this one

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maintaining a library like this will take us a lot of time. cause requirement will have animation, etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zrg-team : Updated with library above

{...iconSpecificStyle}
{...props}
/>
<View ref={ref} style={style}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Icon component is not support ref ? then we have to wrapper ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it has but seem not stable, sometimes it doesn't work and make project breaking. That's why I wrapped it

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok ok. If it not stable good to go. But keep in mind, the library should have fewer components because passing props with many style objects isn't a good idea, you have more work in Jitera tool

@justiniruuza justiniruuza force-pushed the feature/create-carousel-component branch from e258523 to 518ef8b Compare January 21, 2022 08:52
@justiniruuza justiniruuza merged commit f8ce050 into master Jan 21, 2022
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