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

fixed onLayout measurements on web #3019

Merged
merged 8 commits into from
Apr 16, 2024
Merged

Conversation

adids1221
Copy link
Contributor

@adids1221 adids1221 commented Apr 8, 2024

Description

TabBarController fix item size on web.
TabBarItem flex issue, setting flex to 1 on web only if spreadItems is set to true.
Handeling the style of the TabBarItem from TabBar, the style passed via props override it.

Changelog

TabBarController fix item size on web.

Additional info

ticket 4115

@adids1221 adids1221 added the Important for Next Release PR that must be included in the release version label Apr 8, 2024
@adids1221 adids1221 requested a review from ethanshar April 8, 2024 14:23
@adids1221 adids1221 changed the title fixed onLayour measeuremnts on web fixed onLayout measurements on web Apr 9, 2024
@adids1221 adids1221 marked this pull request as draft April 9, 2024 06:56
@adids1221 adids1221 marked this pull request as ready for review April 9, 2024 07:45
@@ -216,6 +217,7 @@ const TabBar = (props: Props) => {
activeBackgroundColor={activeBackgroundColor}
{...item}
{...context}
style={[{flex}, item?.style]}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you memoize this style, it might affect performance if you are passing inline array style

@adids1221 adids1221 requested a review from ethanshar April 9, 2024 09:16
@@ -201,8 +201,16 @@ const TabBar = (props: Props) => {
}
});

const tabBarItemStyle = useMemo(() =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it working?
You are memoizing a function that returns a value and not the value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had an issue, changed it to useCallback and passed the tabBarItemStyle as dependency to the tabBarItems.
Checked on mobile and web.

@adids1221
Copy link
Contributor Author

@ethanshar as we spoke, passing spreadItems to TabBarItem and moved the logic there also.

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.

Wrote a small comment, anyway approved

@@ -8,11 +8,13 @@ import {Colors, Typography, Spacings} from '../../style';
import Badge, {BadgeProps} from '../badge';
import View from '../view';
import TabBarContext from './TabBarContext';
import Constants from '../../commons/Constants';
import {TabControllerBarProps} from './TabBar';
Copy link
Collaborator

Choose a reason for hiding this comment

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

change to import type {TabControllerBarProps} from './TabBar';
It might have a cyclic require because TabBar imports TabBarItem and TabBarItem imports TabBar
Maybe just importing with type is ok

@adids1221 adids1221 merged commit 6cc44e9 into master Apr 16, 2024
1 check passed
@adids1221 adids1221 deleted the web/fix_TabBatItem_onLayout_size branch April 16, 2024 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Important for Next Release PR that must be included in the release version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants