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

Invites: move /accept-invite to /my-sites/invites #1108

Merged
merged 1 commit into from
Dec 1, 2015

Conversation

lezama
Copy link
Contributor

@lezama lezama commented Dec 1, 2015

How to test

Everything in the invite process should still flow.

  • checkout update/move-accept-invite-to-invites
  • invite yourself
  • go to http://calypso.dev:3000/accept-invite/<blog_id>/<invitation_id>

Closes #1101

import InviteHeader from './invite-header';
import LoggedInAccept from './logged-in-accept';
import LoggedOutInvite from './logged-out-invite';
import InviteHeader from 'components/invites/invite-header';
Copy link
Member

Choose a reason for hiding this comment

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

We should avoid app-components in client/components (purer UI components). I'd place invite react components in "my-sites".

@lezama lezama force-pushed the update/move-accept-invite-to-invites branch from 7a88e41 to 7bc488c Compare December 1, 2015 17:13
@lezama lezama changed the title Invites: move /accept-invite to /lib/invites Invites: move /accept-invite to /my-sites/invites Dec 1, 2015
@lezama lezama force-pushed the update/move-accept-invite-to-invites branch 3 times, most recently from 3b6e5f9 to a8775f5 Compare December 1, 2015 17:44
@lezama
Copy link
Contributor Author

lezama commented Dec 1, 2015

cc @ebinnion @roccotripaldi

@lezama lezama added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Dec 1, 2015
@@ -177,6 +172,11 @@
@import 'my-sites/draft/style';
@import 'my-sites/drafts/style';
@import 'my-sites/exporter/style';
@import 'my-sites/invites/invite-form-header/style';
@import 'my-sites/invites/invite-header/style';
@import 'my-sites/invites/invite-accept/logged-in/style';
Copy link
Member

Choose a reason for hiding this comment

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

the logged-in / logged-out things should be prefixed or become sub-components.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't they sub-components?

Copy link
Member

Choose a reason for hiding this comment

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

A subcomponent is invite-accept/logged-in.jsx, the folder logged-in is a full component.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, to clarify, we either need to bring the logged-in and logged-out into the my-sites/invites directory, or do the sub-components?

@lezama lezama force-pushed the update/move-accept-invite-to-invites branch from a8775f5 to 06d3b40 Compare December 1, 2015 19:31
@ebinnion
Copy link
Contributor

ebinnion commented Dec 1, 2015

Code wise, you already addressed my comment it looks like. Functionally, this works fine.

In testing, I found a few bugs, but I don't believe those to be related to this PR and I have created separate issues for them.

:shipit:

@ebinnion ebinnion added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 1, 2015
lezama added a commit that referenced this pull request Dec 1, 2015
…-invites

Invites: move /accept-invite to /my-sites/invites
@lezama lezama merged commit 31def71 into master Dec 1, 2015
@lezama lezama deleted the update/move-accept-invite-to-invites branch December 1, 2015 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants