Skip to content

Feat/basic ui #1038

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

Merged
merged 23 commits into from
Dec 7, 2020
Merged

Feat/basic ui #1038

merged 23 commits into from
Dec 7, 2020

Conversation

mendyEdri
Copy link
Contributor

Description

UI changes and active row logic

Changelog

@mendyEdri mendyEdri requested a review from ethanshar November 18, 2020 12:33
Copy link
Collaborator

@ethanshar ethanshar left a comment

Choose a reason for hiding this comment

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

I wrote in one of the comments about how I approached this component.
Unlike many other components, this one requires a very delicate handling when it comes to performance because the interaction here is very important.
That's the reason I used animated values in many cases.

Animated values are a great way to communicate with the native side without going over the bridge and without updating the state too often.

Go over my comments, we can sync about it if you like.

@mendyEdri mendyEdri requested a review from ethanshar December 3, 2020 12:51
Copy link
Collaborator

@ethanshar ethanshar left a comment

Choose a reason for hiding this comment

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

Looks great!
wrote few comments on naming..

Comment on lines 22 to 23
selectedColor?: string;
unselectedColor?: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

unselected sounds a little weird..
maybe use activeColor and inactiveColor WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree!

Comment on lines 30 to 34
selectedTextColor?: string;
/**
* TextStyle for other, non-focused rows
*/
unselectedTextStyle?: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

same comment here for the names

Comment on lines 56 to 58
if (scrollView.current?.getNode()) {
//@ts-ignore for some reason scrollToOffset isn't recognized
scrollView.current.getNode().scrollToOffset({offset: index * itemHeight, animated: true});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would that work?

Suggested change
if (scrollView.current?.getNode()) {
//@ts-ignore for some reason scrollToOffset isn't recognized
scrollView.current.getNode().scrollToOffset({offset: index * itemHeight, animated: true});
//@ts-ignore for some reason scrollToOffset isn't recognized
scrollView.current?.getNode()?.scrollToOffset({offset: index * itemHeight, animated: true});

@mendyEdri mendyEdri requested a review from ethanshar December 7, 2020 08:29
@ethanshar ethanshar merged commit e3782e9 into feat/WheelPicker Dec 7, 2020
ethanshar added a commit that referenced this pull request Dec 7, 2020
* create a customizable WheelPicker

* Reset PlaygroundScreen changes

* Keep working on WheelPicker (incubator)

* Fix TS issues

* export WheelPicker in incubator index

* Use Flatlist for WheelPicker for better performance when having big lists

* Separate to two examples: months and years

* Feat/basic ui (#1038)

* Removed 'wheel` animation to match the design guidlines + added default colors and font

* added text configuration props

* on selecting item - wheel will scroll to the right row

* added Fader and extract separators

* Calculate current active index

* added onChange event

* Increase visible rows to 5

* change separator color to be visible

* added demo code

* update reanimated version

* types changes

* type update

* extract logic to it's own type

* refactor item center calculation to be based on the offset

* update demo screen according to the new api

* pass style to item

* added unittests to listMiddleIndex calculations

* made demo more clear

* Update src/incubator/WheelPicker/index.tsx

Co-authored-by: Ethan Sharabi <[email protected]>

* PR fixes - naming

* PR fixes - naming

* PR fix - optional chaining

Co-authored-by: Ethan Sharabi <[email protected]>

Co-authored-by: Mendy Edri <[email protected]>
@mendyEdri mendyEdri deleted the feat/basic-ui branch December 14, 2020 09:33
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