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

Api changes #1534

Merged
merged 19 commits into from
Feb 25, 2020
Merged

Api changes #1534

merged 19 commits into from
Feb 25, 2020

Conversation

dmtrKovalenko
Copy link
Member

@dmtrKovalenko dmtrKovalenko commented Feb 23, 2020

This PR is a part of vNext #1293 development

Description

New props

  • toolbarFormat - easily customize date string displaying in toolbar (New property to change the date/time format from the toolbar #1345)
  • disableHighlightToday - disable highlighting of today date with a circle
  • showDaysOutsideCurrentMonth - show days outside of current month in calendar
  • disableMargin - (only Day prop) disable margin between days, useful for range displaying when days are linked together with background color

Remove props

  • labelFunc - completely unusable function with current keyboard input design
  • invalidLabel - pretty same, unusable with keyboard input, use error and helperText instead

Renames

  • format => inputFormat
  • emptyLabel => emptyInputText
  • initialFocusedDate => defaultHighlight
  • title => toolbarTitle (+ make it accept value from label as default)

Change signatures

  • renderDay
(date: DateIOType, selectedDate: DateIOType, dayInCurrentMonth: boolean, defaultDay: React.ReactNode) => React.ReactNode 
(date: DateIOType, selectedDate: DateIOType,  DayComponentProps:  DayProps) => React.ReactNode 

Motivation:

Better customization of the day displaying, by allowing to spread down and override any props we need, before it was possible only by React.cloneElement, but it was not so powerful like spreading props down. This also better from performance perspective when overriding days, because we are not rendering day bides ourself if user will render something different.

  renderDay={(day, selectedDate, DayComponentProps) => {
     return (
        <Badge overlap="circle" badgeContent={isSelected ? '🌚' : undefined}>
          <Day {...DayComponentProps} isToday disableMargin aria-label="Something"  />
        </Badge>
     );
  }}

@vercel
Copy link

vercel bot commented Feb 23, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/mui-org/material-ui-pickers/j765yiwoj
✅ Preview: https://material-ui-pickers-git-feature-api-changes.mui-org.now.sh

@cypress
Copy link

cypress bot commented Feb 23, 2020



Test summary

58 0 1 0


Run details

Project material-ui-pickers
Status Passed
Commit 971bac0
Started Feb 25, 2020 11:28 AM
Ended Feb 25, 2020 11:29 AM
Duration 01:00 💡
OS Linux Debian - 9.8
Browser Electron 61

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@codecov
Copy link

codecov bot commented Feb 24, 2020

Codecov Report

Merging #1534 into next will increase coverage by 0.04%.
The diff coverage is 94.23%.

Impacted file tree graph

@@            Coverage Diff            @@
##            next    #1534      +/-   ##
=========================================
+ Coverage   91.8%   91.85%   +0.04%     
=========================================
  Files         62       62              
  Lines       1746     1755       +9     
  Branches     298      304       +6     
=========================================
+ Hits        1603     1612       +9     
+ Misses       111      109       -2     
- Partials      32       34       +2
Impacted Files Coverage Δ
lib/src/wrappers/MobileWrapper.tsx 100% <ø> (ø) ⬆️
lib/src/views/Calendar/CalendarHeader.tsx 91.52% <ø> (ø) ⬆️
lib/src/DatePicker/DatePicker.tsx 100% <ø> (ø) ⬆️
lib/src/Picker/Picker.tsx 95.23% <100%> (ø) ⬆️
lib/src/DateTimePicker/DateTimePicker.tsx 88.88% <100%> (ø) ⬆️
lib/src/_shared/hooks/usePickerState.ts 87.09% <100%> (ø) ⬆️
lib/src/_shared/PureDateInput.tsx 100% <100%> (ø) ⬆️
lib/src/TimePicker/TimePickerToolbar.tsx 96% <100%> (ø) ⬆️
lib/src/DatePicker/DatePickerToolbar.tsx 96.42% <100%> (+0.42%) ⬆️
lib/src/_helpers/text-field-helper.ts 100% <100%> (+1.21%) ⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0086fbd...971bac0. Read the comment docs.

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 24, 2020

Regarding toolbarTitle, should we call it toolbarText? "Text" being a common suffix we have for strings that should/can be localized?

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Do you keep track of the API breaking changes somewhere? Maybe we should start a migration guide?

@dmtrKovalenko
Copy link
Member Author

I am following material design guidelines, there it is called title, maybe call it toolbarTitleText?
Keeping breaking changes only in releases, just need to combine all releases together.
But yeah starting migration guide is a nice idea, I think maybe we need to provide codemod as well at least for prop renames?

@oliviertassinari
Copy link
Member

I am following material design guidelines, there it is called title, maybe call it toolbarTitleText?

I'm not sure the developers coming to the component will be aware of how the material design specification calls the label. I think that as long as it's intuitive and consistent, it will be great. No strong preference, toolbarTitleText, toolbarText, titleText they sounds great.

codemod as well at least for prop renames?

In terms of user experience, I think that:

  1. a note in each release is a good
  2. a migration guide is great
  3. a codemod is excellent

I think that 2. is a nice tradeoff between the DX and the time required to make it happen. What do you think?

@dmtrKovalenko
Copy link
Member Author

The migration guide will be done anyway. Like we do for v3 https://material-ui-pickers.dev/guides/upgrading-to-v3
I have no experience with codemod scripts so have no idea how much time it will take

@oliviertassinari
Copy link
Member

I have no experience with codemod scripts so have no idea how much time it will take

I would expect a codemod to take about an afternoon of work or more. But I have never done prop renaming for a codemod (at least I only remember doing import renaming). My estimate could be wrong.

@dmtrKovalenko
Copy link
Member Author

I have no idea what's wrong with percy 🤔

@dmtrKovalenko dmtrKovalenko merged commit 9d09c0c into next Feb 25, 2020
@oliviertassinari oliviertassinari deleted the feature/api-changes branch March 18, 2020 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants