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

EPMRPP-79184 || Tooltip component #3316

Merged
merged 9 commits into from
Nov 1, 2022
Merged

Conversation

tr1ble
Copy link
Contributor

@tr1ble tr1ble commented Oct 28, 2022

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Oct 28, 2022

Codecov Report

Merging #3316 (e0b89c0) into develop (81da1dd) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #3316   +/-   ##
========================================
  Coverage    61.65%   61.65%           
========================================
  Files           74       74           
  Lines          798      798           
  Branches       121      121           
========================================
  Hits           492      492           
  Misses         279      279           
  Partials        27       27           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Comment on lines 7 to 8
- **side**: _string_, optional, default = 'top'
- **arrowPosition**: _string_, optional, default = 'left'
Copy link
Contributor

Choose a reason for hiding this comment

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

Side: top little bit confusing. Maybe be better to change to placement or something similar. No need for any action now, let's see the opinion of other team members

@tr1ble tr1ble requested a review from chivekrodis October 28, 2022 14:39
@Vadim73i
Copy link
Contributor

As far as I remember, the position of the arrow should be determined automatically depending on the position of the screen.
Also, there should be support for showing the tooltip in another place if it doesn't fit on the screen in the current one.

@tr1ble
Copy link
Contributor Author

tr1ble commented Oct 31, 2022

As far as I remember, the position of the arrow should be determined automatically depending on the position of the screen. Also, there should be support for showing the tooltip in another place if it doesn't fit on the screen in the current one.

I've shown it to UX and got approve

@tr1ble tr1ble requested a review from Vadim73i October 31, 2022 13:21
@tr1ble
Copy link
Contributor Author

tr1ble commented Oct 31, 2022

Now it's refactored from dynamic (depending on cursor) to static tooltip position depending on element placement on screen

const tooltipRoot = document.getElementById('tooltip-root');
return ReactDOM.createPortal(
<Popper placement={side} modifiers={modifiers} eventsEnabled={false}>
{({ placement, ref, style, arrowProps }) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

Are inline styles prop (style) used?
May be replace with a className?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This style comes from Popper component and it's using for position calucaltion

@tr1ble tr1ble requested a review from Vadim73i November 1, 2022 08:42
@Vadim73i
Copy link
Contributor

Vadim73i commented Nov 1, 2022

Screenshot 2022-11-01 at 2 51 53 PM

Screenshot 2022-11-01 at 3 02 16 PM

There should be padding. Like on popover. Ask the designer if the tooltip should be transparent. And fix the strip on the triangle.

@tr1ble
Copy link
Contributor Author

tr1ble commented Nov 1, 2022

Screenshot 2022-11-01 at 2 51 53 PM Screenshot 2022-11-01 at 3 02 16 PM

There should be padding. Like on popover

Yeah, I've though about it and decided that you can pass offsetLeft and offsetTop to get padding. But I can add default 4px value padding for all cases

@tr1ble
Copy link
Contributor Author

tr1ble commented Nov 1, 2022

Screenshot 2022-11-01 at 2 51 53 PM Screenshot 2022-11-01 at 3 02 16 PM

There should be padding. Like on popover. Ask the designer if the tooltip should be transparent. And fix the strip on the triangle.

On design it has 0.95 opacity.
Strip cannot be fixed even with official Popper example

@Vadim73i
Copy link
Contributor

Vadim73i commented Nov 1, 2022

May be set default value with this padding? This is more useful.
And set side: 'auto' as default.

@tr1ble
Copy link
Contributor Author

tr1ble commented Nov 1, 2022

May be set default value with this padding? This is more useful. And set side: 'auto' as default.

It has 'auto' by default.

@Vadim73i
Copy link
Contributor

Vadim73i commented Nov 1, 2022

May be set default value with this padding? This is more useful. And set side: 'auto' as default.

It has 'auto' by default.

readme says "- side: string, optional, default = 'top'"

@Vadim73i
Copy link
Contributor

Vadim73i commented Nov 1, 2022

Screenshot 2022-11-01 at 2 51 53 PM Screenshot 2022-11-01 at 3 02 16 PM There should be padding. Like on popover. Ask the designer if the tooltip should be transparent. And fix the strip on the triangle.

On design it has 0.95 opacity. Strip cannot be fixed even with official Popper example

But if I change 1 to 0 (withTooltip.scss lines: 93, 113, 134, 155) - it works

@Vadim73i
Copy link
Contributor

Vadim73i commented Nov 1, 2022

And "side: left" shows the tooltip on the right side

@tr1ble
Copy link
Contributor Author

tr1ble commented Nov 1, 2022

May be set default value with this padding? This is more useful. And set side: 'auto' as default.

It has 'auto' by default.

readme says "- side: string, optional, default = 'top'"

Screenshot 2022-11-01 at 2 51 53 PM Screenshot 2022-11-01 at 3 02 16 PM There should be padding. Like on popover. Ask the designer if the tooltip should be transparent. And fix the strip on the triangle.

On design it has 0.95 opacity. Strip cannot be fixed even with official Popper example

But if I change 1 to 0 (withTooltip.scss lines: 93, 113, 134, 155) - it works

I've tried it already, but I'll recheck thanks

@tr1ble
Copy link
Contributor Author

tr1ble commented Nov 1, 2022

Screenshot 2022-11-01 at 2 51 53 PM Screenshot 2022-11-01 at 3 02 16 PM There should be padding. Like on popover. Ask the designer if the tooltip should be transparent. And fix the strip on the triangle.

On design it has 0.95 opacity. Strip cannot be fixed even with official Popper example

But if I change 1 to 0 (withTooltip.scss lines: 93, 113, 134, 155) - it works

Anyway it has strip, not black, but white

@tr1ble
Copy link
Contributor Author

tr1ble commented Nov 1, 2022

And "side: left" shows the tooltip on the right side

Didn't reproduce. And I don't change default logic, we just pass this prop to Popper

@Vadim73i
Copy link
Contributor

Vadim73i commented Nov 1, 2022

And "side: left" shows the tooltip on the right side

Didn't reproduce. And I don't change default logic, we just pass this prop to Popper

Sorry, I didn't fit the tooltip in width and showed on the right. Everything is working.

@AmsterGet AmsterGet merged commit f2e79ba into develop Nov 1, 2022
@AmsterGet AmsterGet deleted the EPMRPP-79184_Tooltip_component branch November 1, 2022 14:11
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

5 participants