-
Notifications
You must be signed in to change notification settings - Fork 482
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
Revert "Revert "Show correct state of sign-in button in dashboard"" #43782
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,14 +12,16 @@ import { | |
refreshProjectName, | ||
setShowTryAgainDialog | ||
} from './headerRedux'; | ||
|
||
import React from 'react'; | ||
import ReactDOM from 'react-dom'; | ||
|
||
import {Provider} from 'react-redux'; | ||
import progress from './progress'; | ||
import {getStore} from '../redux'; | ||
import {asyncLoadUserData} from '@cdo/apps/templates/currentUserRedux'; | ||
import { | ||
setUserSignedIn, | ||
setInitialData | ||
} from '@cdo/apps/templates/currentUserRedux'; | ||
|
||
import {PUZZLE_PAGE_NONE} from '@cdo/apps/templates/progress/progressTypes'; | ||
import HeaderMiddle from '@cdo/apps/code-studio/components/header/HeaderMiddle'; | ||
|
@@ -168,10 +170,44 @@ function setupReduxSubscribers(store) { | |
setupReduxSubscribers(getStore()); | ||
|
||
function setUpGlobalData(store) { | ||
store.dispatch(asyncLoadUserData()); | ||
fetch('/api/v1/users/current', { | ||
credentials: 'same-origin' | ||
}) | ||
.then(response => response.json()) | ||
.then(data => { | ||
store.dispatch(setUserSignedIn(data.is_signed_in)); | ||
if (data.is_signed_in) { | ||
store.dispatch(setInitialData(data)); | ||
ensureHeaderSigninState(true, data.short_name); | ||
} else { | ||
ensureHeaderSigninState(false); | ||
} | ||
}) | ||
.catch(err => { | ||
console.log(err); | ||
}); | ||
} | ||
setUpGlobalData(getStore()); | ||
|
||
// Some of our cached pages can become cached by the browser with the | ||
// wrong sign-in state. This is a temporary patch to ensure that the header | ||
// displays the correct sign-in state for the user. | ||
function ensureHeaderSigninState(isSignedIn, shortName) { | ||
const userMenu = document.querySelector('#header_user_menu'); | ||
const signinButton = document.querySelector('#signin_button'); | ||
|
||
if (isSignedIn && userMenu.style.display === 'none') { | ||
userMenu.style.display = 'block'; | ||
signinButton.style.display = 'none'; | ||
|
||
const displayName = document.querySelector('#header_display_name'); | ||
displayName.textContent = shortName; | ||
} else if (!isSignedIn && signinButton.style.display === 'none') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It strikes me that this would be the default- this is a NIT but it might be organizes as just a default 'let' for those two vars with a single 'if' instead. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I understand what you mean by default 'let' for those two vars. I may follow-up with you on this and get these changes in separately! |
||
userMenu.style.display = 'none'; | ||
signinButton.style.display = 'inline'; | ||
} | ||
} | ||
|
||
header.showMinimalProjectHeader = function() { | ||
getStore().dispatch(refreshProjectName()); | ||
getStore().dispatch(showMinimalProjectHeader()); | ||
|
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 document this somewhere? I can't tell how widespread this application of fetch might be but if we're moving toward fetch over ajax and the default credentials are still different for older browsers I imagine we'll run into this again?
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 definitely agree. One way to handle this would be to not use fetch directly but write our own wrapper around it so that these kind of configurations will be set for it every time it's used. I'll start a conversation about this in apps-devs tomorrow!