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

refactor(DatePicker): Break up, extract handlers, consolidate state #3959

Merged
merged 1 commit into from
Nov 8, 2024

Conversation

m7kvqbe1
Copy link
Collaborator

@m7kvqbe1 m7kvqbe1 commented Nov 7, 2024

Overview

Refactor DatePicker.

This is a boy scout step change, I'm not aiming for perfection (other things to move on to).

I'm also not fully convinced this is better. I feel like i've added more code and indirection.

Reason

#3955 (comment)

Work carried out

  • Break up the DatePicker into Input and FloatingCalendar sub components
  • Extract handlers from JSX
  • Consolidate state within the reducer (calendarTableVariant, isOpen)
  • Expose reducer via context
  • Memoize where appropriate (note impact on speed of test suites)

Developer notes

I'm using context to expose the reducer, which is naughty with mutable state but performance impact is limited:

  1. State updates are naturally bounded to user interactions
  2. Most state changes (like paging the calendar) require child components to re-render anyway
  3. Context scope is limited to the relatively shallow DatePicker component tree only
  4. Refactoring all of the state and introducing Zustand felt like overkill given the above

@m7kvqbe1 m7kvqbe1 added Type: Enhancement New feature or request Package: react-component-library Package/code type labels Nov 7, 2024
@m7kvqbe1 m7kvqbe1 self-assigned this Nov 7, 2024
Copy link

netlify bot commented Nov 7, 2024

Deploy Preview for storybook-navy-digital-mod-uk ready!

Name Link
🔨 Latest commit 349a5b5
🔍 Latest deploy log https://app.netlify.com/sites/storybook-navy-digital-mod-uk/deploys/672de850cac9fd00084fbe7c
😎 Deploy Preview https://deploy-preview-3959--storybook-navy-digital-mod-uk.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@m7kvqbe1 m7kvqbe1 force-pushed the feat/date-picker-refactor branch from 099afaa to d5428c0 Compare November 7, 2024 14:40
@m7kvqbe1 m7kvqbe1 changed the title [WIP] refactor(DatePicker): Break up, extract handlers and consolidate state refactor(DatePicker): Break up, extract handlers and consolidate state Nov 7, 2024
@m7kvqbe1 m7kvqbe1 force-pushed the feat/date-picker-refactor branch from d5428c0 to 77b9691 Compare November 7, 2024 15:28
@m7kvqbe1 m7kvqbe1 requested a review from thyhjwb6 November 7, 2024 15:30
@m7kvqbe1 m7kvqbe1 marked this pull request as ready for review November 7, 2024 15:30
@m7kvqbe1 m7kvqbe1 force-pushed the feat/date-picker-refactor branch from 77b9691 to 2b1f53a Compare November 7, 2024 15:30
@m7kvqbe1 m7kvqbe1 requested a review from jpveooys November 7, 2024 15:30
@m7kvqbe1 m7kvqbe1 changed the title refactor(DatePicker): Break up, extract handlers and consolidate state refactor(DatePicker): Break up, extract handlers, consolidate state Nov 7, 2024
Copy link
Collaborator

@jpveooys jpveooys left a comment

Choose a reason for hiding this comment

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

I think it looks okay generally. Would've been useful to split this into multiple commits though 😄

(Main negative I can see is probably that it's added a fair bit to the line count...)

@m7kvqbe1 m7kvqbe1 force-pushed the feat/date-picker-refactor branch from 8e5cd4d to 5f0ed87 Compare November 8, 2024 09:56
Copy link
Collaborator

@thyhjwb6 thyhjwb6 left a comment

Choose a reason for hiding this comment

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

👍

I think it looks good and also gets me thinking about custom hooks - whether 1-2 more could be introduced or whether the existing ones need rework/adding to, but this is far enough for now.

@m7kvqbe1 m7kvqbe1 force-pushed the feat/date-picker-refactor branch from 5f0ed87 to 349a5b5 Compare November 8, 2024 10:30
@m7kvqbe1 m7kvqbe1 enabled auto-merge November 8, 2024 10:30
Copy link

sonarqubecloud bot commented Nov 8, 2024

@m7kvqbe1 m7kvqbe1 merged commit 953f079 into master Nov 8, 2024
20 checks passed
@m7kvqbe1 m7kvqbe1 deleted the feat/date-picker-refactor branch November 8, 2024 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: react-component-library Package/code type Type: Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants