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

make tooltip arrows work for popovers #370

Merged
merged 3 commits into from
Jul 31, 2019

Conversation

andycochran
Copy link
Contributor

Currently, the arrow styles only work for tooltips. The arrow styles are only targeting .ember-tooltip-arrow. However, popovers do not have this class; they have class="tooltip-arrow ember-popover-arrow". Since both tooltips and popovers have the same .tooltip-arrow class, it can be used instead — nested under the namespace of .ember-tooltip and .ember-popover.

@maxfierke maxfierke self-assigned this Jul 22, 2019
@maxfierke maxfierke added 3.x ember-tooltips Release series 3.x enhancement labels Jul 22, 2019
Copy link
Collaborator

@maxfierke maxfierke left a comment

Choose a reason for hiding this comment

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

Nice catch!

However, I think given that people may be overriding these CSS rules and expecting certain rules to already exist, even when specifying a custom tooltipClass or arrowClass, it would be better to instead fix the popover rules to use ember-popover-arrow, rather than changing them all to use tooltip-arrow. Because ember-tooltip-arrow doesn't work with popovers anyway, this shouldn't cause any problems.

looks like the CI is failing for an unrelated reason, I'll try to fix that up tonight or tomorrow.

@andycochran
Copy link
Contributor Author

I think you're right. Change made. Thanks.

@mockey-jockey
Copy link

mockey-jockey commented Jul 25, 2019

@maxfierke when this change will move to master?

@andycochran thanks for your good fix. for the white background it's not showing properly arrow

image

@mockey-jockey
Copy link

mockey-jockey commented Jul 25, 2019

.ember-tooltip[x-placement^="top"] .ember-tooltip-arrow {
  border-top-color: #3a3c47;
}

@maxfierke can you pls make this as a dynamic also.if i change tooltip background ,background got changed and arrow color not changing.
For example :

.tooltip {
  background: red
}

by this code got changed the background-color

here we are facing an issue is background color is red but border-top-color: #3a3c47

feels like the user does not customize the color for tooltip as they want because of this.

In the document for the custom tooltip, u added display none for an arrow.

can you pls help on this how to resolve this.

@maxfierke
Copy link
Collaborator

I'm traveling this week, but will try to take a look while I'm at the airport later, but it might need to wait until the weekend.

Copy link
Collaborator

@maxfierke maxfierke left a comment

Choose a reason for hiding this comment

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

@andycochran class updates look good to me! @mockey-jockey made a good point about the arrow borders. Do mind updating those border-[right|left|top|bottom]-color to be #ccc to match the default border color for popovers?

CI is now fixed on master too, so if you rebase off master or merge it into your branch, that'll fix the failing check.

@mockey-jockey
Copy link

@maxfierke not only arrow problem in the ember popover it also problem with ember tooltip as well
image

@andycochran do you have any idea for the above image arrow color issue.

when we are giving some custom color to the tooltips this problem occurs.

arrow color only not changing when we gave custom color. pls, suggest some solution.

@andycochran
Copy link
Contributor Author

andycochran commented Jul 29, 2019

@maxfierke I've fixed the popopver arrow color and pulled in master.

As to @mockey-jockey's concerns (which should probably be handled in a separate issue/PR… 

The docs recommend hiding the arrows when giving them custom colors:

  .tooltip-error {
    background-color: #e37f7f;
    color: #fff;
  }

  .tooltip-error .ember-tooltip-arrow {
    display: none;
  }

Instead of the display: none, you could add specific custom styles to color the arrow instead:

.ember-tooltip[x-placement^="top"] .ember-tooltip-arrow {
  border-top-color: #e37f7f;
}

However, you'd have to add a custom style for each side you need the arrow to appear on. I think we could do better.

What if instead of defining border-[side]-color four times, we define border-color once and in each `[x-placement^="[side]"] we override the borders that should be transparent:

.ember-tooltip[x-placement^="top"] .ember-tooltip-arrow {
  border-right-color: transparent !important;
  border-bottom-color: transparent !important;
  border-left-color: transparent !important;
}

Then, custom styles could simply declare matching background and arrow colors:

.tooltip-barbie {
  background-color: Pink;
}

.tooltip-barbie .ember-tooltip-arrow {
  border-color: Pink;
}

Copy link
Collaborator

@maxfierke maxfierke left a comment

Choose a reason for hiding this comment

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

:octocat: lgtm. will probably merge this into master tonight

@maxfierke maxfierke added this to the 3.4.0 milestone Jul 30, 2019
@maxfierke maxfierke merged commit df5ea95 into sir-dunxalot:master Jul 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x ember-tooltips Release series 3.x enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants