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

Light up an additional typescript template check #3518

Merged
merged 4 commits into from
May 10, 2020

Conversation

msorens
Copy link
Contributor

@msorens msorens commented Apr 28, 2020

🔩 Description: What code changed, and why?

Follow-on to Angular 9 template type checking enablement, this throws the switch on one more check--and fixes the problems it reveals.

⛓️ Related Resources

PR #3268 Turn on strict Angular checks

👍 Definition of Done

ng build is clean with an additional check activated

👟 How to Build and Test the Change

Run ng build

✅ Checklist

@msorens msorens requested a review from a team April 28, 2020 02:51
@msorens msorens self-assigned this Apr 28, 2020
@@ -1,5 +1,6 @@
import { Component, OnInit, OnDestroy, EventEmitter } from '@angular/core';
import { FormBuilder, FormGroup, Validators } from '@angular/forms';
import { MatOptionSelectionChange } from '@angular/material/core/option';
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice find!

Copy link
Contributor

@SEAjamieD SEAjamieD left a comment

Choose a reason for hiding this comment

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

Thanks for making all these changes and updates. Happy to see there is a better way than that custom keyboard event 🎉

Copy link
Contributor

@scottopherson scottopherson left a comment

Choose a reason for hiding this comment

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

👍

@msorens msorens force-pushed the ms/add-typescript-check branch 2 times, most recently from 3a63231 to fe1c159 Compare May 1, 2020 20:46
Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

LGTM thanks for doing this

@msorens msorens force-pushed the ms/add-typescript-check branch from fe1c159 to 4531e0d Compare May 10, 2020 02:15
@msorens
Copy link
Contributor Author

msorens commented May 10, 2020

Note to self: this was delayed in merging because of an error that manifested in both automate-ui...

Admin pages
   ✗ when clicked, shows Delete User
       - Failed: No element found using locator: By(css selector, .chef-control-menu mat-option:nth-child(1)

...and in cypress:

1) token management can delete token:
     CypressError: Timed out retrying: Expected to find element: '.chef-control-menu', but never found it.

I had tried to find the cause of those but without much joy. Finally, when I rebased today--a couple weeks later--all tests reported green. Just to make sure it was not a fluke, I reran buildkite again. And again. All passes! So now I am merging.

@msorens msorens merged commit 4506651 into master May 10, 2020
@msorens msorens deleted the ms/add-typescript-check branch May 10, 2020 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants