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

fix(date-picker): add null check for optional date control #1051

Merged

Conversation

twittwer
Copy link
Contributor

@twittwer twittwer commented Nov 9, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • If applicable, have a visual design approval

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

The control property of ClrDateInput's is marked as optional and can therefore be null.

@Self()
@Optional()
protected control: NgControl,

  constructor(
    ...
    @Self()
    @Optional()
    protected control: NgControl,
    ...
  ) {
    super(viewContainerRef, ClrDateContainer, injector, control, renderer, el);
  }

But the WrappedControl which is extended by ClrDateInput doesn't handle nullable controls.

  constructor(
    ...
    private ngControl: NgControl,
    ...
  ) {

This causes TypeError: Cannot read properties of null (reading 'control') when using clrDate and calling markAllAsTouched on a surrounding form.

private markAsTouched(): void {
this.ngControl.control.markAsTouched();
this.ngControl.control.updateValueAndValidity();
}

  private markAsTouched(): void {
    this.ngControl.control.markAsTouched();
    this.ngControl.control.updateValueAndValidity();
  }

What is the new behavior?

The WrappedControl is now typed with nullable control and checks for its existence before using it.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@vmwclabot
Copy link

@twittwer, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

Copy link
Contributor

github-actions bot commented Nov 9, 2023

👋 @kevinbuhmann,

  • 🙏 The Clarity team thanks you for opening a pull request
  • 🎉 The build for this PR has succeeded
  • 🔍 The PR is now ready for review
  • 🍿 In the meantime, view a preview of this PR
  • 🖐 You can always follow up here. If you're a VMware employee, you can also reach us on our internal #clarity-support Slack channel

Thank you,

🤖 Clarity Release Bot

@vmwclabot
Copy link

@twittwer, we have received your signed contributor license agreement. The review is usually completed within a week, but may take longer under certain circumstances. Another comment will be added to the pull request to notify you when the merge can proceed.

@vmwclabot
Copy link

@twittwer, VMware has approved your signed contributor license agreement.

Copy link
Member

@kevinbuhmann kevinbuhmann left a comment

Choose a reason for hiding this comment

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

Thanks!

@twittwer
Copy link
Contributor Author

@kevinbuhmann How would you like to proceed with the failing test?

it('should set the control on NgControlService', () => {
expect(setControlSpy).toHaveBeenCalled();
});

Before it tests that NgControlService.setControl has be called, but not with what. In reality, it was called with null what is not supported by the NgControlService.setControl.

Should I test the opposite and check it is not called, or remove it?

@kevinbuhmann
Copy link
Member

We should have a test for both possibilities.

@twittwer
Copy link
Contributor Author

Ok, I checked it out locally and now lint & test should succeed.

@twittwer
Copy link
Contributor Author

I hope running public-api:update fixes the pipeline

@twittwer
Copy link
Contributor Author

@kevinbuhmann What is required to get this merged & released?

@kevinbuhmann
Copy link
Member

@kevinbuhmann What is required to get this merged & released?

I will get this merged today.

@kevinbuhmann kevinbuhmann merged commit df02084 into vmware-clarity:main Nov 14, 2023
4 checks passed
kevinbuhmann pushed a commit that referenced this pull request Nov 14, 2023
@twittwer twittwer deleted the add-date-control-null-check branch November 15, 2023 11:06
@twittwer
Copy link
Contributor Author

Can you say when the next release will be published?
This is currently blocking our update.
Or is there some next/beta tag to install clarity with the fix?

@twittwer
Copy link
Contributor Author

@kevinbuhmann Can you estimate a time for next release with this fix?

Copy link
Contributor

🎉 This PR is included in version 16.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Copy link
Contributor

Hi there 👋, this is an automated message. To help Clarity keep track of discussions, we automatically lock closed PRs after 14 days. Please look for another open issue or open a new issue with updated details and reference this one as necessary.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants