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

Added offset prop to Tooltip and Popover components #1018

Conversation

illiteratewriter
Copy link
Member

No description provided.

Copy link
Collaborator

@gergely-nagy gergely-nagy left a comment

Choose a reason for hiding this comment

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

LGTM, some small change requests.

  • All changes must have unit tests.
  • Documentation is missing

Copy link
Collaborator

@gergely-nagy gergely-nagy left a comment

Choose a reason for hiding this comment

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

Please fix linter errors

@@ -513,5 +513,17 @@ describe('Popover', () => {

wrapper.detach();
});

it('should pass down offset', ()=> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing space before =>

<Popover
isOpen
target="innerTarget"
offset='100'>Popover content</Popover>
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Unexpected usage of singlequote , use " " instead
  • The closing bracket must be aligned with the line containing the opening tag

offset='100'>Popover content</Popover>
);

expect(wrapper.find(PopperContent).props().offset).toEqual('100')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing semicolon

@@ -236,6 +236,18 @@ describe('Tooltip', () => {
wrapper.unmount();
});

it('should pass down offset', ()=> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing space before =>

<Tooltip
isOpen
target="target"
offset='100'>Tooltip content</Tooltip>
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Unexpected usage of singlequote , use " " instead
  • The closing bracket must be aligned with the line containing the opening tag

offset='100'>Tooltip content</Tooltip>
);

expect(wrapper.find(PopperContent).props().offset).toEqual('100')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing semicolon

@TheSharpieOne
Copy link
Member

Props (within JSX) should use double quotes (just as HTML attributes would be) everything else should be single quotes.

@gergely-nagy
Copy link
Collaborator

gergely-nagy commented May 14, 2018

@illiteratewriter we use Travis-CI to automatically run our unit tests and for code linting.

When a pull request is opened on GitHub (or commits are added to the pull request), Travis CI receives a notification and runs a build.

During the build, Travis CI update the status icon of the pull request to one of the following statuses:

  • a warning that the build is still running.
  • a notification that the build failed – the pull request should not be merged.
  • a notification that the build succeeded – the pull request can be merged.

Your PR build fails, because eslint found some error. You can view the log here.

You should run linter before commits.
To run ESLint all you need to do is run npm run lint from the inside of your project folder.

I hope this helps you. 😃

Copy link
Member

@TheSharpieOne TheSharpieOne left a comment

Choose a reason for hiding this comment

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

LGTM

@gergely-nagy gergely-nagy merged commit 3561e3c into reactstrap:master May 14, 2018
@illiteratewriter
Copy link
Member Author

Thank you for being patient and helping me with this. It was great working with you. :)

@illiteratewriter illiteratewriter deleted the add-offset-to-tooltip-and-popover branch May 18, 2018 08:18
@illiteratewriter illiteratewriter restored the add-offset-to-tooltip-and-popover branch May 18, 2018 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants