KittenSquad/glewlwyd
2
0
Fork 0
Code Issues 6 Pull requests Projects Releases Packages Wiki Activity Actions

Navbar fixes #138

Merged
yrammos merged 2 commits from navbar-fix into master 2020-06-17 16:33:39 +02:00
Conversation 10 Commits 2 Files changed 4 +13 -17
yrammos commented 2020-06-17 11:23:13 +02:00 (Migrated from github.com)
Copy link

The list of users in the two navigation bars (Admin and Profile) is not always synchronized with the actual list of users if two or more users are active. This PR fixes the issue.

The fix consists of two parts. First, only the currently active user is removed from profileList[] (I am avoiding slice() for this purpose because it is considered bad ReactJS practice on array-like states). Second, this.fetchAPI() is no longer called if a user has just logged out.

An edge case producing a crash is also fixed by ensuring that this.state.profileList[0] exist before attempting to index it.

To test, please log in with at least two usernames, then log them out one by one, each time noticing the effects on the navigation bar dropdowns.

The list of users in the two navigation bars (Admin and Profile) is not always synchronized with the actual list of users if two or more users are active. This PR fixes the issue. The fix consists of two parts. First, only the currently active user is removed from `profileList[]` (I am avoiding `slice()` for this purpose because it is considered bad ReactJS practice on array-like states). Second, `this.fetchAPI()` is no longer called if a user has just logged out. An edge case producing a crash is also fixed by ensuring that `this.state.profileList[0]` exist before attempting to index it. To test, please log in with at least two usernames, then log them out one by one, each time noticing the effects on the navigation bar dropdowns.
yrammos commented 2020-06-17 12:52:31 +02:00 (Migrated from github.com)
Copy link

Hmm… the revision solves one problem by creating another (it breaks the green "Continue" button in certain scenarios).

Will revise and reopen. Closing for now.

Hmm… the revision solves one problem by creating another (it breaks the green "Continue" button in certain scenarios). Will revise and reopen. Closing for now.
babelouest commented 2020-06-17 13:44:34 +02:00 (Migrated from github.com)
Copy link

I think this can be solved in another way, please see the code comments

I think this can be solved in another way, please see the code comments
babelouest (Migrated from github.com) reviewed 2020-06-17 13:45:54 +02:00
webapp-src/src/Admin/App.js
@ -106,3 +104,1 @@
if (!this.state.loggedIn) {
this.fetchApi();
}
this.setState({loggedIn: message.loggedIn}, () => {
babelouest (Migrated from github.com) commented 2020-06-17 13:45:54 +02:00
Copy link

you can try by changing the whole block like this:

this.setState({loggedIn: message.loggedIn}, () => {
  this.fetchApi();
});
you can try by changing the whole block like this: ```javascript this.setState({loggedIn: message.loggedIn}, () => { this.fetchApi(); }); ```
babelouest (Migrated from github.com) reviewed 2020-06-17 13:49:09 +02:00
webapp-src/src/Profile/App.js
@ -62,12 +62,8 @@ class App extends Component {
this.setState({curNav: message.module, module: message.page});
}
} else if (message.type === 'loggedIn') {
babelouest (Migrated from github.com) commented 2020-06-17 13:49:08 +02:00
Copy link

Like above, this block can be replaced by this:

this.setState({loggedIn: message.loggedIn}, () => {
  this.fetchProfile();
});
Like above, this block can be replaced by this: ```javascript this.setState({loggedIn: message.loggedIn}, () => { this.fetchProfile(); }); ```
yrammos commented 2020-06-17 14:28:36 +02:00 (Migrated from github.com)
Copy link

@babelouest I found an other, more verbose solution meanwhile, but will now try your more compact suggestion and report back.

@babelouest I found an other, more verbose solution meanwhile, but will now try your more compact suggestion and report back.
babelouest (Migrated from github.com) requested changes 2020-06-17 14:31:37 +02:00
babelouest (Migrated from github.com) left a comment
Copy link

We can still make it more simple

We can still make it more simple
webapp-src/src/Admin/App.js
@ -101,11 +101,8 @@ class App extends Component {
} else if (message.type === 'profile') {
this.fetchApi();
babelouest (Migrated from github.com) commented 2020-06-17 14:30:36 +02:00
Copy link

This isn't required anymore, the this.fetchApi(); below will refresh the user list from the backend, and dispatch the new list to all the subcomponents

This isn't required anymore, the `this.fetchApi();` below will refresh the user list from the backend, and dispatch the new list to all the subcomponents
webapp-src/src/Profile/App.js
@ -62,12 +62,8 @@ class App extends Component {
this.setState({curNav: message.module, module: message.page});
}
} else if (message.type === 'loggedIn') {
babelouest (Migrated from github.com) commented 2020-06-17 14:30:54 +02:00
Copy link

This isn't required anymore, the this.fetchProfile(); below will refresh the user list from the backend, and dispatch the new list to all the subcomponents

This isn't required anymore, the `this.fetchProfile();` below will refresh the user list from the backend, and dispatch the new list to all the subcomponents
babelouest commented 2020-06-17 14:35:42 +02:00 (Migrated from github.com)
Copy link

@yrammos , when you push changes in a PR, please don't push force a rebased branch, simply add new commits on top of the last one, otherwise it's harder to follow up changes.

Also, when a required change is commited, you should use the Resolve conversation button each time, likewise we'll know where we're at without having to re-read all the code changes.

@yrammos , when you push changes in a PR, please don't push force a rebased branch, simply add new commits on top of the last one, otherwise it's harder to follow up changes. Also, when a required change is commited, you should use the `Resolve conversation` button each time, likewise we'll know where we're at without having to re-read all the code changes.
yrammos commented 2020-06-17 15:43:20 +02:00 (Migrated from github.com)
Copy link

@babelouest, sorry about the rebase (symptom of a mostly solo worker).

I implemented your change but the issue with the Admin navbar persisted: the last user to log out would still appear on the navbar. I tracked it down to a tiny glitch with this.fetchApi(). Kindly review for any edge cases I may have not considered.

@babelouest, sorry about the rebase (symptom of a mostly solo worker). I implemented your change but the issue with the *Admin* navbar persisted: the last user to log out would still appear on the navbar. I tracked it down to a tiny glitch with `this.fetchApi()`. Kindly review for any edge cases I may have not considered.
babelouest (Migrated from github.com) approved these changes 2020-06-17 16:08:28 +02:00
babelouest commented 2020-06-17 16:33:08 +02:00 (Migrated from github.com)
Copy link

@yrammos , I've tested the PR and I've seen no edge case or new bugs. I'm pushing as is, thanks!

@yrammos , I've tested the PR and I've seen no edge case or new bugs. I'm pushing as is, thanks!
🎉 1
Sign in to join this conversation.
Reviewers
No reviewers
babelouest
Labels
Clear labels   
bug

   
can't reproduce

   
duplicate

   
help wanted

   
improvement

   
invalid

   
need info

   
question

   
wontfix
No labels
bug
can't reproduce
duplicate
help wanted
improvement
invalid
need info
question
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
KittenSquad/glewlwyd!138
No description provided.