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.

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

[WIP] Refactor TS React #1102

Closed
wants to merge 73 commits into from

Conversation

vincentbavitz
Copy link

Improvements

This PR was requested primarily to implement preliminary modulations in into TypeScript and Redux.
Included also in this PR are the conversation screen UI components rewritten in React that I build in February. The conversation screen now exists in as the parent component ConversationView.tsx which, with Redux.

Efficiency

Restructuring the conversation-view to use React with Redux speeds up message loading considerably in app launch, scrolling and switching between conversations.

Currently, Session stores each opened conversation in its entirety in the DOM. This doesn't scale nicely if the user opens 20 conversations opened from launching, and causes extremely laggy behaviour. Keeping the global Redux store allows us to switch between conversations quickly without storing them in the DOM, and while maintaining state such as scroll position, drafts in composition box, et cetera.

Simplification of Events

We want to be able to trigger certain conversation events from any UI component. For example, when a new LNS mapping is found, we want all references to this name to be instantly and seamlessly updated, no matter which component it's located in.

Currently, backbone collections serve as stores and their corresponding event triggers act in an equivalent manner to reducers. Binding these triggers to our store allows for instantaneous app-wide updates onChange of any collection, without having to send bound methods through complex nested component props.

This also creates a clearer distinction from Signal code and Session code and keeps TypeScript typings strict by removing the intermediary window object in event trigger calls.

For example, this

window.Whisper.events.trigger('showConversation', sessionID);

becomes this

this.props.conversations[id].show();

through this

export function reducer(
  state: ConversationsStateType,
  action: ConversationActionType
): ConversationsStateType { }

UI / UX Improvements

Keyboard navigation

Keyboard navigation cheat-sheet to be added to Session docs.

  • Scroll Up: PageUp, Up Arrow

  • Scroll Down: PageDown, Down Arrow

    Composition Container

    • Send: enter
    • Newline: shift + enter

Microphone Events

Screenshot_20200424_215856

Copy link

@Mikunj Mikunj left a comment

Choose a reason for hiding this comment

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

Also need to run linting

js/background.js Outdated
@@ -1038,8 +1038,9 @@
};

window.toggleMediaPermissions = () => {
const mediaPermissions = window.getMediaPermissions();
window.setMediaPermissions(!mediaPermissions);
// eslint-disable-next-line more/no-then
Copy link

Choose a reason for hiding this comment

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

Don't think this applies

js/conversation_controller.js Outdated Show resolved Hide resolved
MessageCollection: Whisper.MessageCollection,
});

messages = messageSet.models.map(conv => conv.attributes);
Copy link

Choose a reason for hiding this comment

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

Suggested change
messages = messageSet.models.map(conv => conv.attributes);
messages = messageSet.models.map(msg => msg.attributes);

MessageCollection: Whisper.MessageCollection,
});

const message = messageSet.models.map(conv => conv.attributes)[0];
Copy link

Choose a reason for hiding this comment

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

Suggested change
const message = messageSet.models.map(conv => conv.attributes)[0];
const message = messageSet.models.map(msg => msg.attributes)[0];

@@ -2336,6 +2338,8 @@
})
);

console.log(`[vince][unread] Read:`, read);
Copy link

Choose a reason for hiding this comment

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

Forgot to remove this?

let item;
// firstMessageOfSeries tells us to render the avatar only for the first message
// in a series of messages from the same user
item = messageProps ? this.renderMessage(messageProps, message.firstMessageOfSeries) : item;
Copy link

Choose a reason for hiding this comment

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

We should use if-elseif here


return (
<ConversationHeader
id={headerProps.id}
Copy link

Choose a reason for hiding this comment

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

this can all be replaced by

<ConversationHeader {...headerProps} />

// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
public selectMessage(messageId: string) {
const selectedMessages = this.state.selectedMessages.includes(messageId)
// Add to array if not selected. Else remove.
Copy link

Choose a reason for hiding this comment

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

Comment should be // Remove from array if selected else add it so it matches the code

super(props);

// Mouse interaction
this.handleHoverActions = this.handleHoverActions.bind(this);
Copy link

Choose a reason for hiding this comment

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

Same here, you can make the function an arrow function to remove binds



return {
conversations: state.conversations,
Copy link

Choose a reason for hiding this comment

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

Should we pass the current selected conversation here too?

@Mikunj Mikunj marked this pull request as draft April 27, 2020 00:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants