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

Improve User Activity Page Performance #1116

Merged
merged 11 commits into from Nov 29, 2017
Merged

Conversation

jm90m
Copy link
Contributor

@jm90m jm90m commented Nov 26, 2017

Fixes #1105

Changes:

  • Move logic for displaying actions out of component and into actions/reducers
  • Create more separated components that are linked to state to check if they should update
  • Refactor search logic to make it more simpler & faster

* Fixes #1105
* Move logic for displaying actions out of component and into actions/reducers
* Create more separated components that are linked to state to check if they should update
* Refactor search logic to make it more simpler & faster
@jm90m jm90m added the bug label Nov 26, 2017
@bonustrack bonustrack temporarily deployed to busy-master-pr-1116 November 26, 2017 03:39 Inactive
@bonustrack bonustrack temporarily deployed to busy-master-pr-1116 November 26, 2017 05:17 Inactive
@bonustrack bonustrack temporarily deployed to busy-master-pr-1116 November 26, 2017 05:35 Inactive
@@ -91,7 +74,7 @@ class UserActivity extends React.Component {
authenticatedUserName: '',
};

componentDidMount() {
componentWillMount() {
Copy link
Contributor

Choose a reason for hiding this comment

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

componentWillMount is not a place to load async data. componentWillMount triggers on SSR so it wastes resources.

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 yeah good call will change

accountHistoryFilter: PropTypes.arrayOf(PropTypes.string),
userHasMoreActions: PropTypes.bool.isRequired,
loadingMoreUsersAccountHistory: PropTypes.bool.isRequired,
isCurrentUser: PropTypes.bool, // eslint-disable-line react/no-unused-prop-types
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it there if not used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

used in lines 32 and 42, eslint doesn't recognize it :(

class UserActivitySearch extends React.Component {
static propTypes = {
isCurrentUser: PropTypes.bool, // eslint-disable-line react/no-unused-prop-types
Copy link
Contributor

Choose a reason for hiding this comment

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

And there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used in line 130 on this file, eslint didn't recognize it :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used in line 32 and 41 above

@@ -71,7 +71,7 @@ export default class RightSidebar extends React.Component {
<div>
{!authenticated && <SignUp />}
<Switch>
<Route path="/activity" render={() => <UserActivitySearch />} />
<Route path="/activity" render={() => <UserActivitySearch isCurrentUser />} />
Copy link
Contributor

Choose a reason for hiding this comment

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

We can use component instead of render and then we don't need withRouter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call, just updated

@bonustrack bonustrack temporarily deployed to busy-master-pr-1116 November 26, 2017 15:40 Inactive
@bonustrack bonustrack temporarily deployed to busy-master-pr-1116 November 26, 2017 15:45 Inactive
@bonustrack bonustrack temporarily deployed to busy-master-pr-1116 November 26, 2017 17:15 Inactive
@bonustrack
Copy link
Contributor

That look much better, can we have a default margin in the bottom of the content block? Same size than the margin in the bottom of sidebar
image

@bonustrack bonustrack temporarily deployed to busy-master-pr-1116 November 27, 2017 20:09 Inactive
@bonustrack bonustrack temporarily deployed to busy-master-pr-1116 November 27, 2017 20:50 Inactive
@jm90m
Copy link
Contributor Author

jm90m commented Nov 27, 2017

@bonustrack yeah just added the same 16px margin-bottom on it.

@bonustrack bonustrack temporarily deployed to busy-master-pr-1116 November 29, 2017 00:19 Inactive
@jm90m
Copy link
Contributor Author

jm90m commented Nov 29, 2017

@bonustrack @Sekhmet quick look whenever you both get a chance again :)

Copy link
Contributor

@Sekhmet Sekhmet left a comment

Choose a reason for hiding this comment

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

Only thing I've found. Feel free to merge without explicit approval.

totalVestingShares: PropTypes.string.isRequired,
totalVestingFundSteem: PropTypes.string.isRequired,
user: PropTypes.shape().isRequired,
currentDisplayedActions: PropTypes.arrayOf(PropTypes.shape()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Not required props are usually below requires ones (currentDisplayedActions and isCurrentUser).

)
class UserActivityActionsList extends Component {
static propTypes = {
currentDisplayedActions: PropTypes.arrayOf(PropTypes.shape()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Props should be reordered there as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants