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

Migrate to functional components - 1 #1800

Merged
merged 21 commits into from
Mar 2, 2022

Conversation

Inbal-Tish
Copy link
Collaborator

@Inbal-Tish Inbal-Tish commented Feb 21, 2022

private components migration to functional component

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.

@Inbal-Tish
Overall the refactor looks good, But I notice a performance degradation in this PR.

I see that the Basic Day component for example now render each time (all days are being rendered when switching between two days!)

I suggest using this hook to investigate the source of wasteful renders.

In general from a quick look, I see that we pass BasicDay the date prop which seem to be a new object reference each time.
Any reason why shouldn't we pass a day string? (and generate an object internally with date utils)

Also I notice that the Day component accepts a day prop of XDate type. Also here, I think we can use date string instead and internally create an XDate.

I believe that if we'll avoid passing date objects between components we'll see a huge improvement.

src/calendar/day/basic/index.tsx Outdated Show resolved Hide resolved
src/calendar/header/index.tsx Outdated Show resolved Hide resolved
src/calendar/day/index.tsx Outdated Show resolved Hide resolved
Comment on lines 251 to 254
accessibilityActions={[
{name: 'increment', label: 'increment'},
{name: 'decrement', label: 'decrement'}
]}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Avoid passing static inline object, you can define as a file const

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not keeping the object as global file const? it's static

@Inbal-Tish
Copy link
Collaborator Author

this hook

Done and done. Good catch, great hook!

@ethanshar
Copy link
Collaborator

this hook

Done and done. Good catch, great hook!

@Inbal-Tish
I still see performance issues in the first example screen (Calendars)
It might be related to the example, but when I console the renders. The days are still re-render on every day change

src/calendar/day/index.tsx Outdated Show resolved Hide resolved
src/calendar/header/index.tsx Outdated Show resolved Hide resolved
@Inbal-Tish
Copy link
Collaborator Author

Inbal-Tish commented Mar 1, 2022

this hook

Done and done. Good catch, great hook!

@Inbal-Tish I still see performance issues in the first example screen (Calendars) It might be related to the example, but when I console the renders. The days are still re-render on everyday change

Which render do you console? If you're checking Day component then passing onPress is causing a render. I'm working on moving Calendar to functional as well so I can improve this as well. Do you think it's a regression?

@Inbal-Tish Inbal-Tish changed the title Migrate to functional components Migrate to functional components - 1 Mar 2, 2022
@ethanshar
Copy link
Collaborator

Which render do you console? If you're checking Day component then passing onPress is causing a render. I'm working on moving Calendar to functional as well so I can improve this as well. Do you think it's a regression?

Im adding console to the day component.
I thought that we fixed that yesterday, didn't we?

const _isToday = _day ? isToday(_day) : undefined;
const {date, marking, dayComponent, markingType} = props;
const _date = date ? new XDate(date) : undefined;
const _isToday = _date ? isToday(_date) : undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider moving the check for undefined inside isToday

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. I already do. fixing here

@Inbal-Tish
Copy link
Collaborator Author

Which render do you console? If you're checking Day component then passing onPress is causing a render. I'm working on moving Calendar to functional as well so I can improve this as well. Do you think it's a regression?

Im adding console to the day component. I thought that we fixed that yesterday, didn't we?

Yes. Fix was pushed

@Inbal-Tish Inbal-Tish merged commit 1ca84dd into master Mar 2, 2022
@Inbal-Tish Inbal-Tish deleted the infra/migrate_to_functional_components branch April 13, 2022 10:54
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.

None yet

2 participants