Tip - look at marginProp before setting adjustedMargin #7596
Conversation
| @@ -54,7 +54,7 @@ const marginStyle = (theme, align, data, responsive, marginProp) => { | |||
| } | |||
| return edgeStyle( | |||
| 'margin', | |||
| adjustedMargin, | |||
| marginProp || adjustedMargin, | |||
There was a problem hiding this comment.
this way marginProp that is set to 'none' will come before adjustedMargin
There was a problem hiding this comment.
I am comfortable with this. Upon initial review, I was wondering if theme.tip.drop.margin should also adhere to "intelligentMargin" (which would make the margin alignment aware). In looking at the implementation of Tip, "theme.tip.drop" is passed onto Drop as a prop, so allowing for that would further complicate this logic.
Given the use case of Tip with mouseover should ideally be leaving margin as none anyway, I'm comfortable with this.
yes I looked into |
What does this PR do?
This PR address that
tip.drop.marginshould take priority overglobal.drop.margincurrently in our hpe theme we haveglobal.drop.marginset as well asintelligentMarginso whats happening is those are being applied to Tip even though we havetip.drop.marginset to "none"I have add the check that if
marginPropis set to something then that should come before the adjustedMargin in themarginStylefunction in Drop.Where should the reviewer start?
styledDrop
What testing has been done on this PR?
locally
How should this be manually tested?
locally
in hpe theme you will see that the only margin being applied is the
theme.tip.content.marginin which we will update this value in the hpe them to either6pxor9pxdepending on discussion with designers.Do Jest tests follow these best practices?
screenis used for querying.asFragment()is used for snapshot testing.Any background context you want to provide?
marginon the drop was causing the tooltip not to stay open as a user hovered from the target to the tooltipWhat are the relevant issues?
closes #7552
Screenshots (if appropriate)
Do the grommet docs need to be updated?
no
Should this PR be mentioned in the release notes?
yes
Is this change backwards compatible or is it a breaking change?
backwards compatible