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

feat(side-navigation): improve item rendering performance #1449

Merged
merged 8 commits into from Jul 1, 2019

Conversation

@chasestarr
Copy link
Member

commented Jun 24, 2019

Description

Please refer to the scenario I built for some considerations. This pr attempts to provide reasonable memoization to the NavItem component by adding a props comparison function. note that not all props are included for comparison. Changes nested deep in overrides will not queue a re-render. For example if overridden and a state change updates a variable passed to style overrides no change will be visible. Is this acceptable behavior by default? I'm concerned that consumers will not be aware of this - leading to bug reports and this may introduce functionality regressions for users that depend on dynamic overrides. Performing a deep equality comparison on overrides shakes out to similar time perf to always re-rendering items.

Tangentially related, I updated storybook configs so that hooks can be used there.

Scope

  • Minor: New Feature
return true;
}

function compare(prevProps, nextProps) {

This comment has been minimized.

Copy link
@chasestarr

chasestarr Jun 24, 2019

Author Member

I've considered adding a new memoComparison prop to give consumers the ability to bail out of default comparison. What do others think about that approach?

This comment has been minimized.

Copy link
@tajo

tajo Jun 25, 2019

Member

The optimization should be probably opt-in? It's not important for most of applications and brings potential bugs?

This comment has been minimized.

Copy link
@chasestarr

chasestarr Jun 25, 2019

Author Member

👍that's a good idea

if (Object.keys(a).length !== Object.keys(b).length) return false;

for (var key in a) {
if (a[key] !== b[key]) {

This comment has been minimized.

Copy link
@tajo

tajo Jun 25, 2019

Member

should be this recursive / check if a[key] is an object?

chasestarr added some commits Jun 27, 2019

@now

This comment has been minimized.

Copy link

commented Jun 28, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://baseui-documentation-git-side-menu-perf.uber-ui-platform.now.sh

@tajo

tajo approved these changes Jul 1, 2019

chasestarr added some commits Jul 1, 2019

@chasestarr chasestarr merged commit 53c9a0e into master Jul 1, 2019

21 checks passed

Semantic Pull Request ready to be squashed
Details
buildkite/baseui Build #7838 passed (21 minutes, 55 seconds)
Details
buildkite/baseui/docker-package-e2e Passed (7 minutes, 34 seconds)
Details
buildkite/baseui/docker-package-unit Passed (8 minutes, 38 seconds)
Details
buildkite/baseui/documentation-site-link-checker Passed (7 minutes, 14 seconds)
Details
buildkite/baseui/eslint Passed (1 minute, 9 seconds)
Details
buildkite/baseui/flowtype Passed (1 minute, 8 seconds)
Details
buildkite/baseui/fossa Passed (1 minute)
Details
buildkite/baseui/jest Passed (1 minute, 6 seconds)
Details
buildkite/baseui/now Passed (1 minute, 4 seconds)
Details
buildkite/baseui/pipeline Passed (7 seconds)
Details
buildkite/baseui/puppeteer Passed (8 minutes, 47 seconds)
Details
buildkite/baseui/screener Passed (13 minutes, 4 seconds)
Details
ci-gate Pull Request accepted for CI
license/cla Contributor License Agreement is signed.
Details
netlify/baseui/deploy-preview Deploy preview ready!
Details
probot/label-docs-pr Docs label has been set (or unset)
probot/migrations Migration guide provided
probot/pr-label At least one required semver-related label exists
probot/todos All TODOs have open issues
screener Screener visual tests accepted by starr
Details

@baseui-probot-app-workflow baseui-probot-app-workflow bot deleted the side-menu-perf branch Jul 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.