Skip to content

[DatePicker] Fixed rendering issue #2671

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
Merged
merged 4 commits into from
Jan 28, 2020
Merged

[DatePicker] Fixed rendering issue #2671

merged 4 commits into from
Jan 28, 2020

Conversation

AndrewMusgrave
Copy link
Member

@AndrewMusgrave AndrewMusgrave commented Jan 27, 2020 •
edited
Loading

WHY are these changes introduced?

I noticed a "rendering leak" while hovering around the date picker that causes tens of thousands, possibly hundreds with heavy use, of renders.

WHAT is this pull request doing?

Using consistent values (re-structure/memoizing)

Gifs

It's hard to tell since the frame rate is low but I'm hovering around in all these gifs.

With updates

Flame graph with 0 renders 😍

Screen Recording 2020-01-27 at 04 46 PM

Master

A whole lot of renders...

Screen Recording 2020-01-27 at 04 54 PM

Flame graph of renders

Screen Recording 2020-01-27 at 05 01 PM

How to 🎩

  • Load up chroma
  • Head to DatePicker example
  • Hover around and look at the react profiler

🎩 checklist

  • Tested on mobile
  • Tested on multiple browsers
  • Tested for accessibility
  • Updated the component's README.md with documentation changes
  • Tophatted documentation changes in the style guide
  • For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

@github-actions
Copy link
Contributor

github-actions bot commented Jan 27, 2020 •
edited
Loading

🟡 This pull request modifies 5 files and might impact 7 other files. This is an average splash zone for a change, remember to tophat areas that could be affected.

Details:
All files potentially affected (total: 7)
📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/DatePicker/DatePicker.tsx (total: 5)

Files potentially affected (total: 5)

🧩 src/components/DatePicker/components/Day/Day.tsx (total: 7)

Files potentially affected (total: 7)

🧩 src/components/DatePicker/components/Month/Month.tsx (total: 6)

Files potentially affected (total: 6)

🧩 src/components/DatePicker/components/Weekday/Weekday.tsx (total: 7)

Files potentially affected (total: 7)

@AndrewMusgrave AndrewMusgrave marked this pull request as ready for review January 27, 2020 22:12
LauraAubin
LauraAubin reviewed Jan 28, 2020
View reviewed changes
src/components/DatePicker/components/Day/Day.tsx
}

export function Day({
export const Day = memo(function Day({
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL this syntax

@AndrewMusgrave AndrewMusgrave merged commit bebc58f into master Jan 28, 2020
@AndrewMusgrave AndrewMusgrave deleted the fix-datepicker-rendering branch January 28, 2020 21:46
@chloerice chloerice temporarily deployed to production January 30, 2020 17:19 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@LauraAubin LauraAubin LauraAubin approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@AndrewMusgrave @LauraAubin @chloerice