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

Expose position prop in DotTip component #14972

Merged
merged 9 commits into from Jul 11, 2019

Conversation

@HardeepAsrani
Copy link
Contributor

commented Apr 13, 2019

Description

This will expose the position prop in DotTip component which uses Popove component internally to make the popover.

How has this been tested?

It was tested with the default DotTip that are being used for NUX. It's worth mentioning that the positioning will only take place if space is available for it (it drove me mad because I thought it wasn't working).

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

Fixes #14923

HardeepAsrani added some commits Nov 27, 2017

Merge pull request #3 from WordPress/master
Fix CodeEditor not loading when WordPress is installed in a subfolder…
Add position prop to DotTip
Added `position` prop to DotTip component. Closes #14923
@aduth

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

I might like to know if there was some design rationale from the original implementation (#6631, #3670) for which the position would have been hard-coded. Adding "Needs Design Feedback".

@mapk mapk requested a review from karmatosed Jun 25, 2019

@karmatosed

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

I have done some digging into this and the reasoning isn't clear so I think based on that it should be likely to work exposing. I will cc in @noisysocks also as one of the originators of a lot of Tips code.

@noisysocks

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

I have done some digging into this and the reasoning isn't clear so I think based on that it should be likely to work exposing. I will cc in @noisysocks also as one of the originators of a lot of Tips code.

The rationale was that I didn't need it so I didn't add it! 😀

It's worth mentioning that the positioning will only take place if space is available for it (it drove me mad because I thought it wasn't working).

This has bitten me before too! 😂

@@ -33,7 +34,7 @@ export function DotTip( {
return (
<Popover
className="nux-dot-tip"
position="middle right"
position={ position || 'middle right' }

This comment has been minimized.

Copy link
@noisysocks

noisysocks Jul 4, 2019

Member

You could do this with a default argument if you want to make it more explicit that the argument is optional.

export function DotTip( {
	position = 'middle right',
	...
} ) {
	...

Totally up to you, though!

This comment has been minimized.

Copy link
@aduth

aduth Jul 5, 2019

Member

You could do this with a default argument if you want to make it more explicit that the argument is optional.

I'd agree this would probably be the best way to communicate it as an optional prop, and it may help future maintainability if there are changes to the component which reference position in more than this one location.

This comment has been minimized.

Copy link
@HardeepAsrani

HardeepAsrani Jul 8, 2019

Author Contributor

@aduth Should I make this change then?

This comment has been minimized.

Copy link
@aduth

aduth Jul 9, 2019

Member

@aduth Should I make this change then?

It would be a worthwhile improvement if you're willing, yes.

This comment has been minimized.

Copy link
@HardeepAsrani

HardeepAsrani Jul 10, 2019

Author Contributor

@aduth Made the changes. :)

@noisysocks noisysocks merged commit 99a7eb4 into WordPress:master Jul 11, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details
@noisysocks

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

Thanks @HardeepAsrani!

Tug added a commit that referenced this pull request Jul 12, 2019

Expose position prop in DotTip component (#14972)
* Add position prop to DotTip

Added `position` prop to DotTip component. Closes #14923

* Update index.js

jg314 added a commit to jg314/gutenberg that referenced this pull request Jul 19, 2019

Expose position prop in DotTip component (WordPress#14972)
* Add position prop to DotTip

Added `position` prop to DotTip component. Closes WordPress#14923

* Update index.js

@youknowriad youknowriad added this to the Gutenberg 6.2 milestone Jul 26, 2019

sbardian added a commit to sbardian/gutenberg that referenced this pull request Jul 29, 2019

Expose position prop in DotTip component (WordPress#14972)
* Add position prop to DotTip

Added `position` prop to DotTip component. Closes WordPress#14923

* Update index.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.