feat: convert components to use OnPush change detection strategy 2

[Rrothschild18]

Update components to use On Push, part of issue-3066

Changelog

Changed

Dropdown + fix unit tests
File Uploader
Inline loading

[netlify]

✅ Deploy Preview for carbon-angular-next ready!

Name	Link
🔨 Latest commit	25e8dbe
🔍 Latest deploy log	https://app.netlify.com/sites/carbon-angular-next/deploys/67b26c9525f78000081d1443
😎 Deploy Preview	https://deploy-preview-3100--carbon-angular-next.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

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

[netlify]

✅ Deploy Preview for carbon-components-angular ready!

Name	Link
🔨 Latest commit	25e8dbe
🔍 Latest deploy log	https://app.netlify.com/sites/carbon-components-angular/deploys/67b26c9585cd6e0008522938
😎 Deploy Preview	https://deploy-preview-3100--carbon-components-angular.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

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

[Rrothschild18]

The unit tests are using a mock component called DropdownTest as a wrapper to the real Dropdown(cds-dropdown) component.

The unit test below is failing due not trigger change detection correctly. It happens because the wrapper component mutate a specific "non-reactive class property" inside Dropdown component and with changeDetection.Default it would work as intended, but with OnPush this property is inaccessible.

The property menuIsClosed seems to control the state if the dropdown closed or not. Its not a derived state and not specificly bounded to any user event change that would make sense to explicit call marckForCheck() somewhere.

// The failing test
	it("should propagate the change in selected option back to the form and emit a selected event", () => {
		fixture = TestBed.createComponent(DropdownTest); // An angular PseudoApplicationComponentFixture, access to real component 
		wrapper = fixture.componentInstance;
		spyOn(wrapper, "onSelect");
		fixture.detectChanges();
		element = fixture.debugElement.query(By.css("cds-dropdown")); // Typeof DebugElement has only access to the DOM
		element.componentInstance.menuIsClosed = false; // Directly mutating CarbonComponent component class property 
		fixture.detectChanges();
		element.nativeElement.querySelector(".cds--list-box__menu-item__option").click();
		fixture.detectChanges();
		expect(element.nativeElement.querySelector(".cds--list-box__label").textContent).toEqual("one");
		expect(wrapper.onSelect).toHaveBeenCalled();
		expect(wrapper.model.content).toEqual("one");
		expect(wrapper.model.selected).toBe(true);
	});

Solution 1 (implemented) - Update Dropdown "non reactive class property" to be an angular reactive @Input and rely on angular change detection by receiving an new @input reference change. Also updated the docs.

@Component({
	template: `
	<cds-dropdown
                ...
		[menuIsClosed]="menuIsClosed" //
                ...
                >
		<cds-dropdown-list [items]="items"></cds-dropdown-list>
	</cds-dropdown>`
})
class DropdownTest {
        ...
	@Input() menuIsClosed = true; // Add mock @Input
        ...
}

Solution 2 - Re-write all unit tests for not using "wrapper" components since they make testing a little harder with OnPush + class properties that are not @Inputs.

Any other solutions are welcome. Im open to suggestions since the component is big and have few unit tests.

[Akshat55]

I think it might be worth making it an input... I can see use cases of it having it dynamically open/close. However, the naming isn't the best.

Might be better to create a new input (Something like isOpen) and pass data to menuIsClosed using setter? This way angular runs change detection as well.

Then in next official release, we can probably replace all instances of menuIsClosed with the newly created variable. 🤔 Thoughts on this @zvonimirfras.

[zvonimirfras]

isOpen makes more sense than menuIsClosed, for sure.
I don't see a need for a setter when we can just use !isOpen instead. Possibly do vice-versa during deprecation period.

[Rrothschild18]

Rename menuIsClosed to isOpen is way better, it sounded weird for me too. While migrating components im trying to change less code possible.

Ill implement the suggestion of creating an input setter as suggested.

[Rrothschild18]

I've added a few explanations of my thought process.

[Rrothschild18]

.

[Akshat55]

Requires some minor changes, really liked the intro of the menuIsClosed input (To be converted to a setter). Thanks for contributing this @Rrothschild18

[Akshat55]

We can change the following to something like this & update all instaces of menuIsClosed calls to use isOpen.

Suggested change

	@Input() menuIsClosed = true;
	@Input() isOpen = false;
	/** Deprecated as of v6 - Will be removed in v7 */
	set menuIsClosed (isClosed: boolean) {
	this.isOpen = !isClosed;
	this.changeDetectorRef.markForCheck();
	}

[Akshat55]

I still want to run markForCheck here so it doesn't break anything on release. Teams are/could be using ViewChild to update this variable

[Akshat55]

A notice of deprecation should also improve dev experience if they are using it.

[github-actions]

🎉 This PR is included in version 6.0.0-rc.12 🎉

The release is available on:

• npm package (@next dist-tag)
• GitHub release

Your semantic-release bot 📦🚀