Sign-in button is now disabled when token is empty or whitespace #248
Conversation
|
Again, not sure why the build failed. I'm sure I'm doing something wrong since this happened on the last PR as well... |
There was a problem hiding this comment.
👋 Hi @tvand7093! I took a quick scan through the code and noted a few initial questions/suggestions. Would you mind addressing those things? Once that's done, I can take a closer look at this pull request.
Again, not sure why the build failed. I'm sure I'm doing something wrong since this happened on the last PR as well...
I'm not sure why it's failing. I don't think it's due to anything you're doing. I pushed up a branch containing your commit, and the build passed on that branch. 🤔
Can you try merging master into your branch? Maybe that will help. 🤷♂️🤞
lib/sign-in-component.js
Outdated
| @@ -14,6 +14,9 @@ class SignInComponent { | |||
| this.disposables.add(this.props.commandRegistry.add(this.element, { | |||
| 'core:confirm': this.signIn.bind(this) | |||
| })) | |||
| this.props.workspace.observeTextEditors(editor => { | |||
| this.attachChangeEvent() | |||
There was a problem hiding this comment.
Is this needed? This will observe all text editors in the workspace here, and that seems like more than we need.
There was a problem hiding this comment.
It is needed there so that way when the dialog first comes up, the editor is wired up to the event. If this code is commented out, then the button will never be able to change state. I have a test case that demonstrates this. If there is a more appropriate way to handle this, I'm all ears and will address it ASAP!
There was a problem hiding this comment.
It is needed there so that way when the dialog first comes up, the editor is wired up to the event.
We should be able to subscribe to change events right after the call to etch.initialize. [docs]
I think this will do the trick:
diff --git a/lib/sign-in-component.js b/lib/sign-in-component.js
index 2f94c31..62d4d2b 100644
--- a/lib/sign-in-component.js
+++ b/lib/sign-in-component.js
@@ -7,6 +7,12 @@ class SignInComponent {
constructor (props) {
this.props = props
etch.initialize(this)
+
+ this.refs.editor.onDidChange(() => {
+ const token = this.refs.editor.getText().trim()
+ this.refs.loginButton.disabled = !token
+ })
+
this.disposables = new CompositeDisposable()
this.disposables.add(this.props.authenticationProvider.onDidChange(() => {
etch.update(this)
@@ -14,9 +20,6 @@ class SignInComponent {
this.disposables.add(this.props.commandRegistry.add(this.element, {
'core:confirm': this.signIn.bind(this)
}))
- this.props.workspace.observeTextEditors(editor => {
- this.attachChangeEvent()
- })
}
destroy () {
@@ -29,11 +32,8 @@ class SignInComponent {
etch.update(this)
}
- readAfterUpdate () {
- this.attachChangeEvent()
- }
-
render () {
return $.div({className: 'SignInComponent', tabIndex: -1},
$.span({className: 'SignInComponent-GitHubLogo'}),
$.h3(null, 'Sign in with GitHub'),
@@ -78,21 +78,9 @@ class SignInComponent {
: null
}
- attachChangeEvent () {
- const previousTokenEditor = this.editor
- this.editor = this.refs.editor
-
- if (!previousTokenEditor && this.editor) {
- this.editor.onDidChange(() => {
- const token = this.refs.editor.getText().trim()
- this.refs.loginButton.disabled = !token
- })
- }
- }
-
async signIn () {
const token = this.refs.editor.getText().trim()There was a problem hiding this comment.
I’m still new to using etch how so I wasn’t aware of when exactly the refs were available. Learn something new every day!
There was a problem hiding this comment.
You and me both! I just learned this today, too. 😸
test/teletype-package.test.js
Outdated
| @@ -955,6 +955,88 @@ suite('TeletypePackage', function () { | |||
| assert(description.includes('some error')) | |||
| }) | |||
|
|
|||
| test('disables button when empty token specified', async () => { | |||
| { | |||
| const env = buildAtomEnvironment() | |||
There was a problem hiding this comment.
The indentation here appears to be inconsistent with the rest of the code in the repository. Can you please update it to use 2 spaces for indentation.
For style guidelines, please see https://standardjs.com/#the-rules and CONTRIBUTING.md#javascript-styleguide.
There was a problem hiding this comment.
I was following the pattern as seen in tests above where there are multiple cases for one test method.
My bad, I see it now 😉
|
@jasonrudolph as for the build, the only thing I can think of is that I'm doing this on a Mac, so perhaps there is something strange secretly going on... |
|
After looking at a diff between the two build logs is the |
Nice catch. That was indeed the problem. The build is green now. 😅 I'll circle back around to take another look at this pull request 🔜. |
There was a problem hiding this comment.
@tvand7093: Thanks for pushing this forward. I've requested a few changes below. I hope this makes sense. If you have any questions, just let me know. 🙇
lib/sign-in-component.js
Outdated
| @@ -14,6 +14,9 @@ class SignInComponent { | |||
| this.disposables.add(this.props.commandRegistry.add(this.element, { | |||
| 'core:confirm': this.signIn.bind(this) | |||
| })) | |||
| this.props.workspace.observeTextEditors(editor => { | |||
| this.attachChangeEvent() | |||
There was a problem hiding this comment.
It is needed there so that way when the dialog first comes up, the editor is wired up to the event.
We should be able to subscribe to change events right after the call to etch.initialize. [docs]
I think this will do the trick:
diff --git a/lib/sign-in-component.js b/lib/sign-in-component.js
index 2f94c31..62d4d2b 100644
--- a/lib/sign-in-component.js
+++ b/lib/sign-in-component.js
@@ -7,6 +7,12 @@ class SignInComponent {
constructor (props) {
this.props = props
etch.initialize(this)
+
+ this.refs.editor.onDidChange(() => {
+ const token = this.refs.editor.getText().trim()
+ this.refs.loginButton.disabled = !token
+ })
+
this.disposables = new CompositeDisposable()
this.disposables.add(this.props.authenticationProvider.onDidChange(() => {
etch.update(this)
@@ -14,9 +20,6 @@ class SignInComponent {
this.disposables.add(this.props.commandRegistry.add(this.element, {
'core:confirm': this.signIn.bind(this)
}))
- this.props.workspace.observeTextEditors(editor => {
- this.attachChangeEvent()
- })
}
destroy () {
@@ -29,11 +32,8 @@ class SignInComponent {
etch.update(this)
}
- readAfterUpdate () {
- this.attachChangeEvent()
- }
-
render () {
return $.div({className: 'SignInComponent', tabIndex: -1},
$.span({className: 'SignInComponent-GitHubLogo'}),
$.h3(null, 'Sign in with GitHub'),
@@ -78,21 +78,9 @@ class SignInComponent {
: null
}
- attachChangeEvent () {
- const previousTokenEditor = this.editor
- this.editor = this.refs.editor
-
- if (!previousTokenEditor && this.editor) {
- this.editor.onDidChange(() => {
- const token = this.refs.editor.getText().trim()
- this.refs.loginButton.disabled = !token
- })
- }
- }
-
async signIn () {
const token = this.refs.editor.getText().trim()
test/teletype-package.test.js
Outdated
| { | ||
| const env = buildAtomEnvironment() | ||
| const pack = await buildPackage(env, {signIn: false}) | ||
| //this is to trigger the on change event in the token input |
There was a problem hiding this comment.
When you have a moment, would you mind merging the latest code from the master branch into this branch? Once you've done that, you'll see some linter violations reported in the build. Locally, you can run npm run lint to see those violations. Once they're resolved, the build should be green for you again. 😄
test/teletype-package.test.js
Outdated
| assert.equal(message, 'Invalid login token') | ||
| assert(description.includes('The login token must not be empty. Please insert a valid token and try again.')) | ||
| }) | ||
|
|
There was a problem hiding this comment.
test/teletype-package.test.js is intended to provide high-level integration tests for the package as a whole. I like having all these scenarios tested for the sign-in component, but I'd prefer to have most of these scenarios covered in a separate test file that's focused specifically on the behavior of the sign-in component.
With that in mind, I'd love to see a "happy path" integration test in this file, and have the other scenarios live in a file that's more of a unit test for the sign-in component. As an example, there's a high-level integration test for the JoinPortalComponent here:
teletype/test/teletype-package.test.js
Lines 257 to 270 in 282571f
And the more detailed scenarios are covered in more of a unit test fashion here:
teletype/test/portal-list-component.test.js
Lines 89 to 177 in 282571f
Would you be up for taking a similar approach here?
lib/sign-in-component.js
Outdated
| const token = this.refs.editor.getText().trim() | ||
|
|
||
| if (!token) { | ||
| this.props.notificationManager.addError('Invalid login token', { |
There was a problem hiding this comment.
Can we use renderErrorMessage here instead of a notification?
If we use a notification, that notification will appear all the way in the upper-right corner of the app. If we use renderErrorMessage, we can show the error message right next to the token field (i.e., right next to the error 🌟 ).
|
@jasonrudolph thanks for all the feedback! I can for sure take care of these changes. I will probably get to them over the next few evenings (aside from thanksgiving)! 🦃 |
Also refactored some of the fakes into their own files as well.
lib/sign-in-component.js
Outdated
| @@ -31,6 +30,13 @@ class SignInComponent { | |||
| return etch.update(this) | |||
| } | |||
|
|
|||
| readAfterUpdate () { | |||
There was a problem hiding this comment.
Because the view refreshes, we have to rewire the onDidChange event otherwise the sign in button will be stuck in a disabled state, even when a token value is input.
test/teletype-package.test.js
Outdated
|
|
||
| await signInComponent.signIn() | ||
|
|
||
| // TODO: Find out why these refs are not updating. |
There was a problem hiding this comment.
@jasonrudolph: This test case is supposed to test what happens when a token value is provided, the user clicks sign in, and the sign in fails. Testing using atom, it resets the token field and disables the button. However, it seems that I cannot get the refs here to be updated to actually test how it worked. I have banged my head on this all weekend... Any ideas?
|
@tvand7093: Thanks for the updates. I've got this queued up for review. I'll follow up once I've reviewed the latest updates. 🙇 |
|
@tvand7093: Just a quick note to let you know that I haven't forgotten about you. We're wrapping up some big updates, and then I'll circle back around to this pull request. |
To eliminate the need to wire up an observer multiple times [1], always render the login element, but set it to `display: none` when the signing in. Also, fix the failing test [2] by clearing the text when sign-in fails. [1]: atom#248 (comment) [2]: atom#248 (comment)
Use the existing integration test to verify that signing in with an invalid token will disable the sign-in button and clear the token text.
|
@tvand7093: I pushed up a few updates, mostly related to preventing the need for resubscribing to text editor change events each time the popover is re-rendered. 😄 Thanks again for this contribution! 🙇🍻 |
Description of the Change
When first signing in, the sign in button is enabled when there is no value in the token field. This changes makes it so that the sign in button is disabled when:
Once a valid character is typed into the field, the button becomes enabled. This also includes an error prompt in the event someone was able to click the button when the token is empty.
Alternate Designs
These change follow similar changes found here. For this reason, the code is rather similar and not many other decisions were made.
Benefits
This prevents the user from signing in with an empty token, which is more of a standard practice form validation practice.
Possible Drawbacks
N/A
Applicable Issues
#131