-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
fix: Update notification messages for verifying account #5517
fix: Update notification messages for verifying account #5517
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/eventyay/open-event-frontend/91eqs3lgn |
app/templates/verify.hbs
Outdated
@@ -1,9 +1,9 @@ | |||
{{#if this.success}} | |||
<div class="ui {{if this.isLoading 'loading'}} icon info message eight wide column center aligned"> | |||
<p>Your email has been verified successfully!{{#if (not this.session.isAuthenticated)}} Please login to continue. {{/if}}</p> | |||
<p>your email has been verified successfully! You now have full access to all feature.{{#if (not this.session.isAuthenticated)}} Please login to continue. {{/if}}</p> |
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.
English sentences start with a capital letter
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.
Please use:
Your account is unverified. Please verify it by clicking on the confirmation link that has been emailed to you.
app/templates/verify.hbs
Outdated
@@ -1,9 +1,9 @@ | |||
{{#if this.success}} | |||
<div class="ui {{if this.isLoading 'loading'}} icon info message eight wide column center aligned"> | |||
<p>Your email has been verified successfully!{{#if (not this.session.isAuthenticated)}} Please login to continue. {{/if}}</p> | |||
<p>your email has been verified successfully! You now have full access to all feature.{{#if (not this.session.isAuthenticated)}} Please login to continue. {{/if}}</p> |
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.
all is used for plurals
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.
Please use:
Thank you, your email has been verified successfully! You now have full access to all features.
@iamareebjamal @mariobehling sir, i have deleted /verify route as it will be of no use if we want to display message at front page. I am still working on this. One thing is still missing. After registration of new user, email have verification link as localhost:4200/verify?token=abcdefcghtuheru_. This route is removed in this PR. screenshots |
That's not what we want. Please revert |
@iamareebjamal sir, instead of showing message on top of front page. You need front page content in this white space. |
@iamareebjamal sir, can u tell be little bit more about desired result, so that i can think in right direction. Do we want front page content in white space area?? |
It will definitely be of use, so please revert this |
456fffe
to
4bb205c
Compare
4bb205c
to
5175264
Compare
@iamareebjamal @mariobehling sir, i have revert my changes. |
@iamareebjamal sir, have a look at this. |
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.
Build is failing
Please always fix the build before asking for reviews. |
message-at-front-page Revert "message-at-front-page" This reverts commit 456fffe. messages-update
5175264
to
e4f0465
Compare
@iamareebjamal sir, done. Build successful. |
@iamareebjamal sorry sir, it left opened in browser. I have closed. |
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.
need to update tests.
open-event-frontend/tests/integration/components/unverified-user-message-test.js
Lines 37 to 44 in 6d509ce
test('unverified message', async function(assert) { | |
setShouldShowMessage.call(this, { | |
currentRouteName: 'else' | |
}); | |
this.set('isMailSent', false); | |
await render(hbs`{{unverified-user-message session=session authManager=authManager isMailSent=isMailSent}}`); | |
assert.dom(this.element).includesText('Your account is unverified. Please verify by clicking on the confirmation link that has been emailed to you.'); | |
}); |
@snitin315 thank you, i am updating tests. |
@iamareebjamal sir, test also updated. please review. |
Not until build passes |
Codecov Report
@@ Coverage Diff @@
## development #5517 +/- ##
===============================================
- Coverage 23.76% 23.70% -0.06%
===============================================
Files 498 498
Lines 5256 5256
Branches 44 44
===============================================
- Hits 1249 1246 -3
- Misses 4001 4004 +3
Partials 6 6
Continue to review full report at Codecov.
|
Fixes #5516
Short description of what this resolves:
update notification mesaages for verifying account.
Frist 2 points (changes) have done in this PR.
Working on third point(change).
screenshots
Checklist
development
branch.