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

[WIP]MM-9686: Fix/async channel view #981

Closed
wants to merge 1 commit into from
Closed

[WIP]MM-9686: Fix/async channel view #981

wants to merge 1 commit into from

Conversation

sudheerDev
Copy link
Contributor

@sudheerDev sudheerDev commented Mar 20, 2018 •
edited
Loading

Summary

Change channel view to be dependent on user/channel object async call.

Related Issue
Related ticket

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • Ran make check-style to check for style errors (required for all pull requests)
  • Ran make test to ensure unit and component tests passed
  • Added or updated unit tests (required for all new features)
  • Has server changes (please link)
  • Has redux changes (please link)
  • Has UI changes
  • Includes text changes and localization file (.../i18n/en.json) updates
  • Touches critical sections of the codebase (auth, posting, etc.)

@jwilander jwilander added the Work in Progress Not yet ready for review label Mar 20, 2018
@sudheerDev sudheerDev mentioned this pull request Mar 20, 2018
Merged
2 tasks
@sudheerDev sudheerDev changed the title Fix/async channel view [WIP]: Fix/async channel view Mar 20, 2018
@sudheerDev sudheerDev force-pushed the fix/asyncChannelView branch 2 times, most recently from 6e63c89 to e891ec8 Compare March 20, 2018 18:05
@koxen
Copy link
Contributor

koxen commented Mar 23, 2018

While I personally use only one team currently, I am looking forward to these changes 👍

@sudheerDev
Copy link
Contributor Author

sudheerDev commented Mar 23, 2018 •
edited
Loading

@koxen This PR improves the performance for loading post and the perceived difference is more for channels than teams so, you might notice it as well :)

With network throttle here is before and after comparison(Subjected to change though)

mar-24-2018 01-15-17

mar-24-2018 00-58-34

@koxen
Copy link
Contributor

koxen commented Mar 23, 2018 •
edited
Loading

i don't see at what point this code could improve performance of in-team channel switching. could you explain?

koxen
koxen reviewed Mar 23, 2018
View reviewed changes
components/channel_layout/channel_identifier_router.jsx Outdated
import {joinChannel} from 'mattermost-redux/actions/channels';
import {getUser, getUserByUsername, getUserByEmail} from 'mattermost-redux/actions/users';
import {getTeamByName} from 'mattermost-redux/selectors/entities/teams';
import $ from 'jquery';
Copy link
Contributor

Choose a reason for hiding this comment

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

please replace with pattern used here https://github.com/mattermost/mattermost-webapp/blob/master/components/permalink_view/permalink_view.jsx#L37

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is WIP. Please dont review it yet!. It might be waste of time as it is subject for change. Not to worry it will not have jquery API's though.

Copy link
Contributor

@koxen koxen Mar 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

sorry for reviewing too early :) thanks for acknowledging the ban on adding new jquery code to project 🙅‍♂️

@sudheerDev
Copy link
Contributor Author

@koxen I posted about this in pre-release. Calling loadPosts API on a parent component give the perf benefit

@koxen
Copy link
Contributor

koxen commented Mar 23, 2018 •
edited
Loading

but these calls are async anyway? so if the channel is loaded in memory, it will be rendered instantly and loadPosts is called async to refresh data

@sudheerDev
Copy link
Contributor Author

@koxen Yes, that is the reason in both the screenshots above I posted with a just loaded instance of the window. I shouldn't have steered the discussion on perf here, my bad. I might not even consider those changes for final PR though.

@sudheerDev sudheerDev force-pushed the fix/asyncChannelView branch 4 times, most recently from 258286c to 2b77150 Compare April 3, 2018 09:56
@sudheerDev sudheerDev changed the title [WIP]: Fix/async channel view [WIP]MM-9686: Fix/async channel view Apr 3, 2018
@jasonblais
Copy link
Contributor

@sudheerDev Are there any next steps on this one?

@sudheerDev
Copy link
Contributor Author

@jasonblais Just started working on this. Need to address the review comments and add test cases. This would require a thorough testing as well as it touches some critical parts of the app. Faster in the release cycle the better.

@sudheerDev
Copy link
Contributor Author

Closing this in favor of #1352.

@jasonblais I will be splitting this into a couple of PR's

  1. Fix duplicate calls of post API and async channel view.
  2. Call post API higher in call stack for faster loading.

@sudheerDev sudheerDev closed this Jun 19, 2018
@jasonblais
Copy link
Contributor

@sudheerDev Sounds good 👍

@lindalumitchell lindalumitchell added the Tests/Not Needed Does not require new release tests label Jun 20, 2018
hmhealey pushed a commit that referenced this pull request Aug 28, 2020
@avegrv @hmhealey
Fix dependencies for react native app #978 (#981)
cfb598e
hmhealey pushed a commit that referenced this pull request Mar 17, 2021
@avegrv @hanzei
Fix dependencies for react native app #978 (#981)
4e3dedd
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@koxen koxen koxen left review comments

Assignees
No one assigned
Labels
Tests/Not Needed Does not require new release tests Work in Progress Not yet ready for review
Projects
None yet
Milestone
No milestone
5 participants
@sudheerDev @koxen @jasonblais @jwilander @lindalumitchell