Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Decide on which screen to show after login in one place #743

Merged
merged 3 commits into from
Mar 14, 2017

Conversation

lukebarnard1
Copy link
Contributor

@lukebarnard1 lukebarnard1 commented Mar 8, 2017

This follows from a small amount of refactoring done when RTS was introduced. Instead of setting the screen after sync, do it only after login.

This requires element-hq/element-web#3385

This includes:

  • initialScreenAfterLogin, which can be used to set the screen after login, and represents the screen that would be viewed if the window.location at the time of initialising Riot were routed.
  • guestCreds are now part of state, because otherwise they don't cause the login/registration views to update when set.
  • instead of worrying about races and using this._setPage, use a dispatch.

Also, I found a erroneous onTeamMemberRegistered, which isn't a thing anymore.

Attempts to fix element-hq/element-web#2337 and similar woes.

Luke Barnard added 2 commits March 8, 2017 10:25
This follows from a small amount of refactoring done when RTS was introduced. Instead of setting the screen after sync, do it only after login.

This requires as-yet-to-be-PRd riot-web changes.

This includes:
 - initialScreenAfterLogin, which can be used to set the screen after login, and represents the screen that would be viewed if the window.location at the time of initialising Riot were routed.
 - guestCreds are now part of state, because otherwise they don't cause the login/registration views to update when set.
 - instead of worrying about races and using this._setPage, use a dispatch.
}

dis.dispatch({action: 'view_room_directory'});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would vote for ditching the returns here and putting this in an else.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, fair enough

initialScreenAfterLogin: React.PropTypes.shape({
screen: React.PropTypes.string.isRequired,
params: React.PropTypes.object,
}),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about this, what we probably ought to do is just have a prop called screen (or maybe currentScreen?) that the parent uses to control the currently displayed screen (re-rendering when it changes), and ditch showScreen which calls methods on React components which is frowned upon, apparently. This is a step in the right direction, then.

Copy link
Contributor Author

@lukebarnard1 lukebarnard1 Mar 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IRL we discussed that this would require wrapping a component around MatrixChat so that it does less routing itself. The thing wrapping it would be a view controller listening for view_... actions that it would respond to by setting a currentScreen prop on MatrixChat. We're de-scoping this for now though. (element-hq/element-web#3386)

@lukebarnard1 lukebarnard1 assigned dbkr and unassigned lukebarnard1 Mar 10, 2017
@dbkr dbkr merged commit 30b4425 into develop Mar 14, 2017
martindale pushed a commit to FabricLabs/matrix-react-sdk that referenced this pull request Dec 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

#/login link doesn't work on riot.im page
2 participants