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.

Sign up for GitHub

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

[No QA] Small refactors. #2744

Merged
merged 4 commits into from
May 13, 2021
Merged

[No QA] Small refactors. #2744

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6db8d76
chg: removed unnecessary re-render & helps in animations.
parasharrajat May 7, 2021
3c4833f
made condition easier to read
parasharrajat May 7, 2021
8f0dbc4
recfactor Tooltip
parasharrajat May 7, 2021
99101e0
updated props type
parasharrajat May 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 10 additions & 23 deletions src/components/Tooltip/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ class Tooltip extends PureComponent {
super(props);

this.state = {
// Is tooltip rendered?
isReady: false,
parasharrajat marked this conversation as resolved.
Show resolved Hide resolved

// The distance between the left side of the wrapper view and the left side of the window
xOffset: 0,

Expand All @@ -38,7 +41,6 @@ class Tooltip extends PureComponent {
this.animation = new Animated.Value(0);

this.getWrapperPosition = this.getWrapperPosition.bind(this);
this.measureWrapperAndGetPosition = this.measureWrapperAndGetPosition.bind(this);
this.measureTooltip = this.measureTooltip.bind(this);
this.showTooltip = this.showTooltip.bind(this);
this.hideTooltip = this.hideTooltip.bind(this);
Expand Down Expand Up @@ -92,25 +94,6 @@ class Tooltip extends PureComponent {
}));
}

/**
* Measure the size and position of the wrapper view.
*
* @param {Object} nativeEvent
*/
measureWrapperAndGetPosition({nativeEvent}) {
const {width, height} = nativeEvent.layout;

// We need to use `measureInWindow` instead of the layout props provided by `onLayout`
// because `measureInWindow` provides the x and y offset relative to the window, rather than the parent element.
this.getWrapperPosition()
.then(({x, y}) => this.setStateIfMounted({
wrapperWidth: width,
wrapperHeight: height,
xOffset: x,
yOffset: y,
}));
}

/**
* Measure the size of the tooltip itself.
*
Expand All @@ -127,6 +110,9 @@ class Tooltip extends PureComponent {
* Display the tooltip in an animation.
*/
showTooltip() {
if (!this.state.isReady) {
this.setState({isReady: true});
}
this.animation.stopAnimation();
this.shouldStartShowAnimation = true;

Expand Down Expand Up @@ -174,7 +160,7 @@ class Tooltip extends PureComponent {
tooltipTextStyle,
pointerWrapperStyle,
pointerStyle,
} = getTooltipStyles(
} = this.state.isReady ? getTooltipStyles(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary? We are not using any of these styles anyway unless this.state.isReady === true.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's better to just move this style calculations to TooltipRenderedOnPageBody as they are only used there.

this.animation,
this.props.windowWidth,
this.state.xOffset,
Expand All @@ -185,9 +171,10 @@ class Tooltip extends PureComponent {
this.state.tooltipHeight,
_.result(this.props, 'shiftHorizontal'),
_.result(this.props, 'shiftVertical'),
);
) : {};
return (
<>
{this.state.isReady && (
<TooltipRenderedOnPageBody
animationStyle={animationStyle}
tooltipWrapperStyle={tooltipWrapperStyle}
Expand All @@ -198,14 +185,14 @@ class Tooltip extends PureComponent {
measureTooltip={this.measureTooltip}
text={this.props.text}
/>
)}
<Hoverable
containerStyle={this.props.containerStyle}
onHoverIn={this.showTooltip}
onHoverOut={this.hideTooltip}
>
<View
ref={el => this.wrapperView = el}
onLayout={this.measureWrapperAndGetPosition}
style={this.props.containerStyle}
>
{this.props.children}
Expand Down
2 changes: 1 addition & 1 deletion src/pages/home/report/ReportActionsView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ class ReportActionsView extends React.Component {
const previousLastSequenceNumber = lodashGet(lastItem(prevProps.reportActions), 'sequenceNumber');
const currentLastSequenceNumber = lodashGet(lastItem(this.props.reportActions), 'sequenceNumber');
const shouldRecordMaxAction = Visibility.isVisible()
&& !(this.props.isDrawerOpen && this.props.isSmallScreenWidth);
&& (!this.props.isSmallScreenWidth || !this.props.isDrawerOpen);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please add a comment here and list out every situations where we will record the max action?


if (previousLastSequenceNumber !== currentLastSequenceNumber) {
// If a new comment is added and it's from the current user scroll to the bottom otherwise
Expand Down