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

fixing modal visibility issue #7191

Closed
wants to merge 1 commit into from
Closed

fixing modal visibility issue #7191

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 17, 2016

As a user viewing my stream, when I click on "You can search or invite more contacts.", the modal backdrop appears but the modal content is invisible. This is because the element containing the modal in the sidebar is hidden until the "invite" section is uncollapsed.

This PR moves the modal to :after_content.

@svbergerem
Copy link
Member

Nice catch! Could you add a test for this? One similar to this test would be great, where you just click on the other link to open the invitation modal.

Copy link
Member

@svbergerem svbergerem left a comment

Choose a reason for hiding this comment

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

I just had two minor remarks.

Please squash your commits so we only have one commit at the end. Putting different behavior changes in different commits is a great thing but in this case it is just one change and a test for it. As long as you are still working on something you may of course have commits fixing/extending stuff from earlier commits.

@@ -49,6 +49,15 @@ Feature: Invitations
Then I should have 1 Devise email delivery
And I should not see "change your notification settings" in the last sent email

Scenario: sends an invitation from a page link
Copy link
Member

Choose a reason for hiding this comment

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

sends an invitation from the stream

@@ -49,6 +49,15 @@ Feature: Invitations
Then I should have 1 Devise email delivery
And I should not see "change your notification settings" in the last sent email

Scenario: sends an invitation from a page link
When I sign in as "[email protected]"
And I click on selector "a.invitations-link"
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer And I click on "invite" within "#no_contacts". In the sidebar we also have the invitations-link class but in that case it is a div. This could be confusing.

@svbergerem
Copy link
Member

Merged as c7a0b05. Thank you!

@svbergerem svbergerem added this to the 0.6.2.0 milestone Nov 17, 2016
@Flaburgan
Copy link
Member

Thanks and congrats for these first pull requests :D

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.

2 participants