Add a logged in class to EmbeddedPage and react to MatrixClient changes #3066
Conversation
See element-hq/element-web#9957 The two hacks introduced here are for different reasons, mostly related to the welcome page. If you land directly on the welcome page, the app's lifecycle is highly unlikely to have a bootstrapped client. This results in the loggedIn class being false. When the client is later set up (loaded from session, new guest account registered, etc) the context fails to update for the EmbeddedPage, and we need to give it a kick to re-render. It's arguable if we should even keep using the context here.
turt2live
added
the
X-Release-Blocker
| render() { | ||
| const client = this.context.matrixClient; | ||
| // HACK: Workaround for the context's MatrixClient not updating. | ||
| const client = this.context.matrixClient || MatrixClientPeg.get(); |
There was a problem hiding this comment.
Yeah, if it's easier, feel free to not use the context here.
There was a problem hiding this comment.
I think I'll leave it for compatibility concerns, and next time I swing by the component I'll take an axe to it.
| const isGuest = client ? client.isGuest() : true; | ||
| const className = this.props.className; | ||
| const classes = classnames({ | ||
| [className]: true, | ||
| [`${className}_guest`]: isGuest, | ||
| [`${className}_loggedIn`]: !!client, |
There was a problem hiding this comment.
This is fine, but it does make me wonder if we should have a very high level class about logged in set on some top level component in the app? Then the welcome page and anything else can use CSS based on that one class, if it wants to, rather than sprinkling logged in classes to specific components like this. Anyway, up to you.
There was a problem hiding this comment.
I'm fairly uncomfortable with the amount of indirection already here (it's not obviously clear that these classes are available). Putting them higher feels like a recipe for subtle and nasty bugs.
See element-hq/element-web#9957
The two hacks introduced here are for different reasons, mostly related to the welcome page. If you land directly on the welcome page, the app's lifecycle is highly unlikely to have a bootstrapped client. This results in the loggedIn class being false. When the client is later set up (loaded from session, new guest account registered, etc) the context fails to update for the EmbeddedPage, and we need to give it a kick to re-render. It's arguable if we should even keep using the context here.