The Wayback Machine - https://web.archive.org/web/20190826053129/https://github.com/WordPress/gutenberg/pull/14972
Skip to content

/gutenberg

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
+10 −1

Conversation

@HardeepAsrani
Copy link
Contributor

commented Apr 13, 2019 •
edited by swissspidy

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

@HardeepAsrani
Merge pull request #1 from WordPress/master 
Update
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
f2facd3
@HardeepAsrani
Merge pull request #2 from WordPress/master 
Merge changes
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
28f415f
@HardeepAsrani
Merge pull request #3 from WordPress/master 
Fix CodeEditor not loading when WordPress is installed in a subfolder…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
b36a918
@HardeepAsrani
Merge pull request #4 from WordPress/master 
Update the git
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
9c1aeae
@HardeepAsrani
Merge pull request #5 from WordPress/master 
Merge
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
ced75ae
@HardeepAsrani
Merge pull request #6 from WordPress/master 
Rebase
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
3a7b078
@HardeepAsrani
Merge pull request #7 from WordPress/master 
Merge updates
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
8703e13
@HardeepAsrani
Add position prop to DotTip 
Added `position` prop to DotTip component. Closes #14923
Loading status checks…
5dc9656

@HardeepAsrani HardeepAsrani requested review from aduth, ajitbohra, gziolo, jorgefilipecosta, noisysocks, talldan and youknowriad as code owners Apr 13, 2019

@HardeepAsrani HardeepAsrani referenced this pull request Apr 14, 2019
Closed

@youknowriad youknowriad added Good First Review NUX labels Apr 15, 2019

@aduth

This comment has been minimized.

Sign in to view
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".

@aduth aduth added the Needs Design Feedback label Apr 24, 2019

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

@karmatosed

This comment has been minimized.

Sign in to view
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.

Sign in to view
Copy link
Member

commented Jul 4, 2019 •
edited

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! 😂
@noisysocks

noisysocks approved these changes Jul 4, 2019
View changes

packages/nux/src/components/dot-tip/index.js Outdated
@@ -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.

Sign in to view
Copy link
@noisysocks

noisysocks Jul 4, 2019 •
edited

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.

Sign in to view
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.

Sign in to view
Copy link
@HardeepAsrani

HardeepAsrani Jul 8, 2019

Author Contributor

@aduth Should I make this change then?

This comment has been minimized.

Sign in to view
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.

Sign in to view
Copy link
@HardeepAsrani

HardeepAsrani Jul 10, 2019

Author Contributor

@aduth Made the changes. :)
@HardeepAsrani
Update index.js
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
bb52b48

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

1 check passed

Travis CI - Pull Request Build Passed
Details
@noisysocks

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jul 11, 2019

Thanks @HardeepAsrani!

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

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

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

* Update index.js
3964f9c

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

@HardeepAsrani @jg314
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
368cbe7

@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

@HardeepAsrani @sbardian
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
26464a5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@noisysocks noisysocks

@karmatosed karmatosed

@youknowriad youknowriad

@talldan talldan

@gziolo gziolo

@ajitbohra ajitbohra

@aduth aduth

@jorgefilipecosta jorgefilipecosta

Assignees
No one assigned
Labels
Good First Review NUX Needs Design Feedback
Projects
None yet
Milestone
  Gutenberg 6.2
5 participants
@HardeepAsrani @aduth @karmatosed @noisysocks @youknowriad
You can’t perform that action at this time.