Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Profile tab and UI for setting away status #3267

Closed
wants to merge 10 commits into from
Closed

Profile tab and UI for setting away status #3267

wants to merge 10 commits into from

Conversation

borisyankov
Copy link
Contributor

@borisyankov borisyankov commented Jan 11, 2019 •
edited
Loading

Based on #3263

Includes a new Profile card, and UI for setting 'away' and 'status text'.

@borisyankov borisyankov added the P0 critical Highest priority label Jan 24, 2019
@borisyankov borisyankov force-pushed the profile-card branch 3 times, most recently from bb2c4cc to 53b80c4 Compare January 28, 2019 23:06
gnprice
gnprice reviewed Jan 29, 2019
View reviewed changes
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks @borisyankov !

I just merged one of these commits, as 6ac0bf2.

Some comments below on the other changes. One meta-comment:

  • The urgent part is to have some way of setting the away status, as well as to consume it.
  • This PR looks like it's going in a direction that may involve a more complicated restructuring of a part of our UI and navigation.
  • I worry that the more complex restructuring may not get to something we can merge this week. I'd recommend focusing on the parts that are required for a v1 of away status, keeping that as simple as possible, and getting it to the point it can be merged.

src/users/userSelectors.js Outdated
getAllUsersByEmail,
getOwnEmail,
(usersByEmail, ownEmail) => usersByEmail[ownEmail],
);
Copy link
Member

Choose a reason for hiding this comment

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

This is one of those selectors that we get no material benefit from caching, because its body does so little work (in fact it's probably more work to check and maintain the cache than to just execute the body). So I'd much rather simplify by leaving out createSelector:

export const getOwnUser = (state: GlobalState): User =>
  getAllUsersByEmail(state)[getOwnEmail(state)];

src/users/userSelectors.js Outdated
@@ -54,6 +54,12 @@ export const getUsersSansMe: Selector<User[]> = createSelector(
(users, ownEmail) => users.filter(user => user.email !== ownEmail),
);

export const getOwnUser: Selector<User[]> = createSelector(
Copy link
Member

Choose a reason for hiding this comment

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

This type is bogus -- this returns a User, not a User[].

Note that Flow says the body of this selector is uncovered, which is why that was possible. You can fix that by putting an appropriate type on getAllUsersByEmail.

src/account-info/OwnAccountCard.js Outdated
@@ -0,0 +1,44 @@
/* @flow */
Copy link
Member

Choose a reason for hiding this comment

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

All new files should be strict-local. (In the absence of a good reason otherwise... but I think that will almost always end up as a reason to put something like a $FlowFixMe in a specific place. With a comment saying why, as always for fixmes.)

In this case, adding that marking causes Flow to point out the props: Object.

src/account-info/UserStatusCard.js Outdated
handleSetUserStatus = () => {
const { dispatch } = this.props;
dispatch(tryStopNotifications());
dispatch(logout());
Copy link
Member

Choose a reason for hiding this comment

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

I'm going to assume this is one of the WIP areas. 😉

@borisyankov borisyankov force-pushed the profile-card branch 10 times, most recently from 642e115 to ca5104c Compare February 4, 2019 20:26
@borisyankov borisyankov added the blocked on other work To come back to after another related PR, or some other task. label Feb 5, 2019
@gnprice gnprice removed the blocked on other work To come back to after another related PR, or some other task. label Feb 6, 2019
@gnprice
Copy link
Member

gnprice commented Feb 6, 2019

And #3263 is merged!

@borisyankov borisyankov force-pushed the profile-card branch 10 times, most recently from a33880d to cfe6cb4 Compare February 8, 2019 22:01
@borisyankov
Copy link
Contributor Author

The first 5 commits are ready for review (one is the same as the other PR)
The rest could be reviewed but are still being changed a bit.

@borisyankov borisyankov force-pushed the profile-card branch 13 times, most recently from 32659b3 to 226c6db Compare February 14, 2019 16:52
@borisyankov borisyankov changed the title WIP: UI for setting away status Profile tab and UI for setting away status Feb 14, 2019
@borisyankov
Add OwnAvatar component
6bca6b8 
A component very similar to the existing `Avatar`.
It is simpler and differs in these ways:
 * automatically extracts the currently logged user
 * does not render TextAvatar as it is only for group conversations
 * does not have `onPress`
@borisyankov
Add AwayStatusSwitch component
266fe58 
This is a stand-alone component that:
 * retrieves the current user's `user status` data and presents it
 * allows by switching it to control the `away` status
@borisyankov
Add ProfileCard component
8e566c1 
This is very similar to `AccountDetails` but used to show the
current users account.

Adding `SwitchAccountButton` and `LogoutButton` buttons there as
a natural place for them to be in.
@borisyankov
Use ProfileCard in MainTabs
5cd64e5 
Adds the new `ProfileCard` as a tab to `MainTabs`, the icon is the
person's avatar.

The end-goal is to replace the Settings tab completely but we will
do that change after shipping the User Status feature.
@borisyankov
Remove 'Switch and Logout buttons from Settings`
81db7fb 
They were placed there not because it made too much sense (they are
not a configuration option) but because we did not have a Profile
tab/screen. Now we do, so they are removed.
@borisyankov
Rework AccountDetails to be more generic
1e414c4 
Move "Send private message" out of `AccountDetails`.
This allows us to reuse `AccountDetails` in `ProfileCard` but not
show the "Send private message" button that is not appropriate there.

This will still keep the option if a user sees their own account
via the (i) button that shows `AccountDetailsScreen`.
@borisyankov
Add UserStatusScreen
21c0d6d
@borisyankov
Add Set status button in ProfileCard
142f926
@borisyankov
Add getAvatarFromUser and getAvatarFromMessage
dbfcb1a 
We duplicate the same trivaial code for extracting an avatar's url
either from a User object or a Message object.

This code extracts that into two utility functions and replaces the
duplicate code with function calls. DRY.
@borisyankov
WIP: Optimistic status updates
ee13faf
@borisyankov
Copy link
Contributor Author

Closing as the original review was done on 4 commits that are very outdated compared to what is currently in. Opening #3344

@gnprice gnprice mentioned this pull request Feb 15, 2019
Merged
@gnprice gnprice added the a-presence/status Presence, and "away status" label Mar 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gnprice gnprice gnprice left review comments

Assignees
No one assigned
Labels
a-presence/status Presence, and "away status" P0 critical Highest priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@borisyankov @gnprice