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

Release rc.10 #457

Merged
merged 70 commits into from
Jun 13, 2018
Merged

Release rc.10 #457

merged 70 commits into from
Jun 13, 2018

Conversation

dmtrKovalenko
Copy link
Member

@dmtrKovalenko dmtrKovalenko commented Jun 8, 2018

We apologize for long break between releases. But we have done a lot inside ⭐️

Big thanks to 6 contributors, that makes this release possible ❤️
Here is what's changed:

Breaking changes

We have redone forwarding refs logic with the new React's api. So no more pickerRef prop

<DatePicker
- pickerRef={node => { this.picker = node }}
+ ref={node => { this.picker = node }}
/>

Features

Fixes

@dmtrKovalenko dmtrKovalenko self-assigned this Jun 8, 2018
@dmtrKovalenko
Copy link
Member Author

@cherniavskii will you have any chance to briefly review it? Thank you

}

static defaultProps = {
minDate: '1900-01-01',
maxDate: '2100-01-01',
disablePast: false,
disableFuture: false,
allowKeyboardControl: false,
Copy link
Member

Choose a reason for hiding this comment

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

looks like it's also breaking change - it was enabled by default

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually its not a breaking change - because its enabled in the DatePickerWrapper.
The logic is - if you are using default wrapper with modal you have events disabled. If you are using some internal component - you got its disabled

Copy link
Member

Choose a reason for hiding this comment

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

I've got confused with DatePicker and DatePickerWrapper components once again :/
Okay, it's not a breaking change.

Copy link
Member Author

Choose a reason for hiding this comment

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

:D I think we should rename them in the next iteration

okLabel: PropTypes.string.isRequired,
cancelLabel: PropTypes.string.isRequired,
clearLabel: PropTypes.string.isRequired,
okLabel: PropTypes.node.isRequired,
Copy link
Member

Choose a reason for hiding this comment

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

hmm, okLabel is used as aria-label in Button component. Same with other labels. Wouldn't that break aria-labels?

@baig
Copy link
Contributor

baig commented Jun 8, 2018

I've added support for seconds in DateTimePicker as well. I will open another pull request for that.

@mcMickJuice
Copy link
Contributor

mcMickJuice commented Jun 11, 2018

@dmtrKovalenko I'm noticing a couple bugs with develop, both affecting DatePicker:

Cannot select other dates

When the calendar is open, today's date (or whatever value is) is selected, but you cannot select other dates.

I updated #459 with fixes for these changes since these issues are all tightly related.

autoOk flag is not respected

If autoOk is true, selecting a date does not close the modal. This is because we're not getting a ref to ModalWrapper to call close on it.

Separate Issue related to date initialization

I think we need to have a separate method in BasePicker for initializing the date of a picker when it's opened. Calling handleChange from componentDidMount in Calendar is specific enough. OR we augment getValidDateOrCurrent in BasePicker to handle setting initial date value.

I prefer the latter since for #450 will also likely require messing with getValidDateOrCurrent for setting the default date if no value is passed:

const getValidDateOrCurrent = ({ utils, value, initialFocusedDate }) => {
+  const initialDate = value || initialFocusedDate || utils.date();

-  const date = utils.date(value);
+  const date = utils.date(initialDate);

- return utils.isValid(date) && value !== null ? date : utils.date();
+  return utils.isValid(date) ? date : utils.date();
};

@dmtrKovalenko
Copy link
Member Author

@mcMickJuice thank you, your PR fixes all of these issues?

@mcMickJuice
Copy link
Contributor

mcMickJuice commented Jun 11, 2018

Nope, sorry here are the DatePicker related issues and checked are what my PR fixes:

Based on looking at the code in BasePicker and cursory testing, the autoOk issue is affecting all components (DatePicker, DateTimePicker, TimePicker).

@dmtrKovalenko
Copy link
Member Author

I think we are ready to release 🚀

@@ -66,6 +67,17 @@ export default class ModalWrapper extends PureComponent {
open: false,
}

static getDerivedStateFromProps(nextProps) {
// only if accept = true close the dialog
if (nextProps.isAccepted) {
Copy link
Contributor

Choose a reason for hiding this comment

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

So you're handling opening/closing of Modal declaratively, using isAccepted as the signal to close, correct? As opposed to imperatively using a ref to ModalWrapper?

Copy link
Member Author

Choose a reason for hiding this comment

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

@mcMickJuice you are exactly right

Copy link
Contributor

Choose a reason for hiding this comment

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

cool! thanks

@dmtrKovalenko dmtrKovalenko merged commit 844796d into master Jun 13, 2018
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.

6 participants