-
Notifications
You must be signed in to change notification settings - Fork 0
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
[#21] [#8] As a user, I can login with an email and password #35
base: develop
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for moonlit-buttercream-05cb2f ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
fe714c3
to
8b55683
Compare
694e8e8
to
ceffe70
Compare
8f0e4e8
to
102e630
Compare
…functionality wip
…ntirely of application
…ubmit event instead button click
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 get not found page when trying to access login path on preview site. Not sure, if I am doing something wrong 🤔
src/screens/Login/index.tsx
Outdated
|
||
navigate('/'); | ||
} catch (error) { | ||
let errorMessage = 'There was a problem receiving a response from the server'; |
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.
Should we add this base error message to the translation file?
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.
Yup, forgot that one :)
Added here 61d68f0
src/adapters/baseAdapter..ts
Outdated
return requestManager('GET', `${BaseAdapter.baseUrl}/${endpoint}`, params); | ||
} | ||
|
||
postRequest(endpoint: string, params: RequestParamsType) { |
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.
Should this be request option to be more accurate with the request manager?
postRequest(endpoint: string, params: RequestParamsType) { | |
postRequest(endpoint: string, requestOption: RequestParamsType) { |
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 used the naming conventions from Compass
so I think should be default, but I can go with this as well
edit: changed here 9d0540f 👍
@hanam1ni Yes, I noticed this too so I waited until merged to main to see. I think this will resolve the issue: Works now, I made following changes: 9538bf5. It involved creating a _redirects file. Login page: https://63fedb58e0e03327cdf00509--moonlit-buttercream-05cb2f.netlify.app/login Edit: Woops, login page doesn't send request correctly as client id and secret not in environment, or the URL. I added to netlify but not sure I need in workflow too, dont think so. |
2e47cf0
to
fb04b03
Compare
<Input | ||
name="email" | ||
label={t('login.email')} | ||
type="text" |
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.
why don't we use type="email"
🤔 or we want to use custom validation here?
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.
yeah I didn't want the html5 default errors to appear on the page for the control (which I think is what would happen). What do you think? Is it not the preferred approach?
src/screens/Login/index.tsx
Outdated
className="my-3 block h-14 w-80" | ||
onInputChange={handleEmailChange} | ||
/> | ||
<div className="relative w-80"> |
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.
Maybe we don't need w-80
as we already set the width of the password input itself 💭
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.
This comment and previous rectified here 👍
<> | ||
<img className="inline-block" src={logo} alt="logo" /> | ||
<p data-test-id="login-header" className="my-8 text-white opacity-50"> | ||
{t('login.sign-in')} to Nimble |
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.
Should we extract the whole text for translation? 🤔 (including to Nimble)
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 answered that in a previous PR - #33 (comment)
My thinking was that Nimble
is the company name and I don't think we should "translate" the company name? I think it would be the same in all languages? Let me know if you think otherwise :)
398c3d8
to
5059f71
Compare
… cypress tests have API data
5059f71
to
71150ac
Compare
What happened 👀
access_token
andrefresh_token
from response in LocalStorageProtecting survey pages and redirecting to homepage has been separated to another issue here: #25
Maintaining a session by obtaining a new access token with refresh token been moved to separate ticket here: #36
Insight 📝
localStorage
mock due to being client wide library and unable to be mocked naturally by Jestnock
package for API request mocking for login page and AuthAdaptersetupTests
to set the values forenvironment
variables in testsProof Of Work 📹
Tests pass