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.

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

Hds 1168 fix dateinput perf #909

Merged
merged 15 commits into from
Dec 21, 2022
Merged

Hds 1168 fix dateinput perf #909

merged 15 commits into from
Dec 21, 2022

Conversation

minevala
Copy link
Contributor

@minevala minevala commented Dec 7, 2022 •
edited
Loading

Description

  • Upgrade popperjs libraries
  • Render DatePicker only when it is opened
  • Update DatePicker snapshot tests

Related Issues

Motivation and Context

  • DateInput renders DatePicker always, even if it is not opened. This might cause some performance issues because DatePicker is rendered when the overall form state changes.

How Has This Been Tested?

  • locally on dev machine
  • functional component tests

Demo

@minevala minevala requested a review from a team December 7, 2022 16:00
@minevala minevala marked this pull request as ready for review December 8, 2022 16:49
@minevala
Copy link
Contributor Author

minevala commented Dec 8, 2022 •
edited
Loading

Warning from Tooltip tests:

Warning: flushSync was called from inside a lifecycle method. React cannot flush when React is already rendering. Consider moving this call to a scheduler task or micro task.

Needs some investigation

@minevala minevala changed the title Hds 1168 fix dateinput perf WIP: Hds 1168 fix dateinput perf Dec 12, 2022
@minevala
Downgrade react-popper
792a640 
Latest version is not compatible with React 17
@minevala
Copy link
Contributor Author

minevala commented Dec 13, 2022 •
edited
Loading

Warning from Tooltip tests:

Warning: flushSync was called from inside a lifecycle method. React cannot flush when React is already rendering. Consider moving this call to a scheduler task or micro task.

Needs some investigation

I had to downgrade react-popper because the latest version is not probably compatible with React 17
floating-ui/react-popper#445

@minevala minevala changed the title WIP: Hds 1168 fix dateinput perf Hds 1168 fix dateinput perf Dec 13, 2022
minevala
minevala commented Dec 14, 2022
View reviewed changes
packages/react/src/components/dateInput/components/datePicker/DatePicker.tsx Outdated
@@ -300,11 +303,18 @@ export const DatePicker = (providedProps: DayPickerProps) => {

const currentMonthAvailableDays: number[] = currentMonthAvailableDates.map((date) => date.getDate());

const onPopperRender = React.useCallback(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

onPopperReady

minevala
minevala commented Dec 14, 2022
View reviewed changes
packages/react/src/components/dateInput/components/datePicker/DatePicker.tsx Outdated
@@ -76,6 +76,8 @@ export const DatePicker = (providedProps: DayPickerProps) => {
*/
const [selectedDate, setSelectedDate] = useState<Date>(selected || null);

const [isReadyToShow, setIsReadyToShow] = useState<boolean>(false);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

isPopperReady, setIsPopperReady

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • rename constants

minevala
minevala commented Dec 14, 2022
View reviewed changes
packages/react/src/components/dateInput/components/datePicker/DatePicker.tsx Outdated
// Initialize Popper.js
const { styles: datePickerPopperStyles, attributes: datePickerPopperAttributes } = usePopper(
inputRef.current,
pickerWrapperRef.current,
{
onFirstUpdate: onPopperRender,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Try Popper lifecycle methods

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • use lifecycle methods

minevala
minevala commented Dec 14, 2022
View reviewed changes
packages/react/src/components/dateInput/components/datePicker/DatePicker.tsx Outdated
// Initialize Popper.js
const { styles: datePickerPopperStyles, attributes: datePickerPopperAttributes } = usePopper(
inputRef.current,
pickerWrapperRef.current,
{
onFirstUpdate: onPopperRender,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add explanatory comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • add comment

harriplappalainen
harriplappalainen approved these changes Dec 19, 2022
View reviewed changes
Copy link
Contributor

@harriplappalainen harriplappalainen left a comment

Choose a reason for hiding this comment

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

Good fixes to tests and needless rendering 🏅

@minevala minevala merged commit 6d17a57 into master Dec 21, 2022
@minevala minevala deleted the hds-1168-fix-dateinput-perf branch December 21, 2022 07:49
@harriplappalainen harriplappalainen restored the hds-1168-fix-dateinput-perf branch July 7, 2023 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harriplappalainen harriplappalainen harriplappalainen 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.
2 participants
@minevala @harriplappalainen