remove some popper arrow styles and target tippy-arrow#268
remove some popper arrow styles and target tippy-arrow#268RobbieTheWagner merged 4 commits intomasterfrom
Conversation
BrianSipple
left a comment
BrianSipple
left a comment
There was a problem hiding this comment.
This all LGTM, @chuckcarpenter. I thinking changing our approach to theming is something we can tackle in 3.0 -- perhaps just starting by leveraging Tippy's themes directly.
docs/welcome/js/welcome.js
Outdated
| ` | ||
| ], | ||
| attachTo: '.hero-welcome bottom', | ||
| attachTo: '.hero-welcome bottom-start', |
There was a problem hiding this comment.
There was a problem hiding this comment.
Also, I think we still need to add positioning calculations for -start and -end.
There was a problem hiding this comment.
We've got a few styling issues here. I agree with @BrianSipple that we need to remove where we are creating popper arrows. We need to remove the borders/drop shadows from the arrows here as well.
There was a problem hiding this comment.
@chuckcarpenter @rwwagner90 Perhaps it would be best to simply add to our existing Popper arrow styles for now (i.e., adding calculations for the -start and -end positions).
We could make fully converting to Tippy arrows a part of 2.X or 3.0.
There was a problem hiding this comment.
@BrianSipple I'm okay with either, but it might be simpler to fully convert to tippy arrows now, rather than trying to use the old styles. I'll leave it up to you guys 😃
There was a problem hiding this comment.
@BrianSipple I think this is easier than adding the additional calculations though? This is a setting and falls back to Tippy options.
There was a problem hiding this comment.
Adding the additional calculations would mean we don't risk breaking things like the positioning of the main content and aesthetic appearance of the arrows. Ultimately, using Tippy directly would be cleaner, but I feel like we'd be better off focusing on that after shipping 2.0.
There was a problem hiding this comment.
@chuckcarpenter @rwwagner90 Setting the position to bottom-start still produces the screenshot above.
There was a problem hiding this comment.
@chuckcarpenter @rwwagner90 This could be what we want, though. I was thinking the placement should move the arrow, but after revisiting both the Tippy and Popper demos, it appears that the main idea is to move the body of the tooltip.
BrianSipple
left a comment
BrianSipple
left a comment
There was a problem hiding this comment.
If we're no longer using it, we should probably remove the part of our code that's creating a popper arrow
|
@BrianSipple Ch ch changes... |
RobbieTheWagner
left a comment
RobbieTheWagner
left a comment
There was a problem hiding this comment.
This seems more or less okay to me. We still need to clean up the theming and arrows at some point, but if this is good with you guys, it's good with me 👍
|
I'm good with this if I'm right that I was wrong here |
|
@BrianSipple your link isn't linking to what you were wrong about for me. Can you elaborate? If you and @chuckcarpenter agree that this is ready, one of you can feel free to merge. We should quickly follow up with extensive arrow and theming refactors though, probably. |
|
@rwwagner90 It's the discussion where I requested changes. I thought that the styling of the tooltip was an issue -- but maybe it's not. |
|
@BrianSipple yeah that link still doesn't work. Perhaps a bug in GitHub, but it doesn't show the comment. @BrianSipple @chuckcarpenter Just to reiterate, if you guys both agree this is good to go, feel free to merge. I just want to make sure that immediately after this we switch to using tippy arrows and themes directly, instead of doing a bunch of custom styles. |
|
I think it's ready, then we can iterate further. Do you agree @BrianSipple ? |
3 participants
Fixes #266