-
Notifications
You must be signed in to change notification settings - Fork 2k
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
People: Invites: Add accepted invite welcome message #636
Conversation
This approach seems fine to me 👍 Although, note that there are some formatting issues currently where the notice is going behind the sidebar. This may get fixed in #302 though, so let's keep an eye on that PR. I think we're going to run into issues with logged out users since these users won't be able to see the Calypso dashboard. So, how do we handle users that turn decline invites or users that opt to only follow by email? cc @rickybanister |
c612464
to
d9d87e9
Compare
@@ -1,7 +1,14 @@ | |||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should place all of these non-view pieces be in lib
.
4702a31
to
15258df
Compare
@mtias could you give it another look, I changed everything :p I would love some help on how to tackle the design
I think we should move the entire |
I'm not sure I agree to moving the entire directory to within |
@ebinnion consider the large folder groups in client (my-sites, reader, me, signup, post-editor) as a way to organize the main areas of the apps and their views. There are some miscellaneous pieces (translator, layout components, etc) that don't quite fit in any of these buckets. Perhaps this is one of them? I'd like for us to find a place for the react components used here that is not the root of client. We've had on our minds the idea of reworking the Now thinking a bit more about this, |
} | ||
return ( | ||
<SimpleNotice status="is-success" onClick={ dismissInviteAccepted }> | ||
<h3 className="invite-message__title">{ this.translate( 'You\'re now a user of: %(site)s', { args: { site: site.slug } } ) }</h3> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the site should be a link to the front-end. Also the sentence would read better without the :
— You're now a user of <a>Blabla</a>.
, with a period at the end.
Sure, but please, open an issue for it so we don't lose track — I hope we avoid adding stuff into client's root in general. |
cc @rickybanister for a design review. |
@@ -2,6 +2,8 @@ | |||
* Internal dependencies | |||
*/ | |||
import wpcom from 'lib/wp' ; | |||
import Dispatcher from 'dispatcher'; | |||
import { DISPLAY_INVITE_ACCEPTED_NOTICE, DISMISS_INVITE_ACCEPTED_NOTICE, DISPLAY_INVITE_DECLINED_NOTICE, DISMISS_INVITE_DECLINED_NOTICE } from './invite-message/constants' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oooh, I love this. Great idea!! I'm going to copy it :D
/** | ||
* Internal dependencies | ||
*/ | ||
import SimpleNotice from 'notices/simple-notice' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think SimpleNotice have just been (like... 2 hours ago) deprecated! :/
We need to update this for using instead of , but once that is done, I think code-wise this is ready to go |
15258df
to
4189d7b
Compare
…tices People: Invites: Add accepted invite welcome message
How to partially test
add/invites-accept-decline-notices
calypso.localhost:3000/?invite_accepted=<blog_id>
You should see a message like this:
Partially-closes #133
cc @ebinnion @roccotripaldi for an early review