Skip to content

fix: Web-Share code-Back tooltip appears at LHN top corner #19826

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

Merged
PauloGasparSv merged 3 commits into from
May 31, 2023
Merged

fix: Web-Share code-Back tooltip appears at LHN top corner #19826

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a380eff
fix: Web-Share code-Back tooltip appears at LHN top corner
kamaljitSharma21 May 30, 2023
a7dc67a
fix: Web-Share code-Back tooltip appears at LHN top corner on safari
kamaljitSharma21 May 31, 2023
fa8116b
fix: linting error
kamaljitSharma21 May 31, 2023
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
3 changes: 3 additions & 0 deletions src/components/Tooltip/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ class Tooltip extends PureComponent {
* @param {Object} bounds - updated bounds
*/
updateBounds(bounds) {
if (bounds.width === 0) {

@parasharrajat parasharrajat Jun 9, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we add a comment explaining the purpose of this code?

@parasharrajat parasharrajat Jun 9, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
if (bounds.width === 0) {
if (!bounds.width) {

@PauloGasparSv PauloGasparSv Jun 9, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we add a comment explaining the purpose of this code?

I also thought of asking for a comment here but the line looks pretty self-explanatory no? If there is no width in the modal it means it shouldn't be rendered right?

@parasharrajat parasharrajat Jun 9, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The purpose of the function updateBounds was only to save the bounds. This is clearly a side-effect. Now I think we should move this condition somewhere else.

Let's watch out for these side-effects and try to keep pure functions clean. cc: @sobitneupane

@PauloGasparSv PauloGasparSv Jun 9, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The purpose of the function updateBounds was only to save the bounds

Ummm, that makes total sense : )

This is clearly a side-effect. Now I think we should move this condition somewhere else.

@parasharrajat pretty sure you understand this much better than I do so can you help me understand what problems does that line cause?

Also, I'm not sure where else to add this condition if we really want to move this logic.

@parasharrajat parasharrajat Jun 10, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should go to componentDidUpdate for class components. Or we can rename this function if this place is convenient.

@kamaljitSharma21 kamaljitSharma21 Jun 12, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@parasharrajat @PauloGasparSv as updateBounds is the place where we can apply bounds.width check because here we have direct access to this variable. So I think instead of dealing with multiple side effects like componentDidUpdate and updateBounds we can go with the suggestion of @parasharrajat to rename this function, which will keep the rendering logic of the tooltip in one place.

@PauloGasparSv PauloGasparSv Jun 12, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Or we can rename this function if this place is convenient.

we can go with the suggestion of @parasharrajat to rename this function, which will keep the rendering logic of the tooltip in one place.

Great, let's please change that function name then :D

Tks again for the help and explanation @parasharrajat

@PauloGasparSv PauloGasparSv Jun 12, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since everything related to this issue is closed and paid out and the whole review process is a bit lengthy I think we should rename that in a new P.R. but not necessarily create a new one.

I'll try to fix this next time I work on an App P.R. since it's not a priority.

this.setState({isRendered: false});
}
this.setState({
wrapperWidth: bounds.width,
wrapperHeight: bounds.height,
Expand Down