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.

Sign up for GitHub

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

Jump to bottom

feat(material/tooltip): add isDisabled method to tooltip harness #27038

Merged
merged 1 commit into from
May 8, 2023
Merged

feat(material/tooltip): add isDisabled method to tooltip harness #27038

merged 1 commit into from
May 8, 2023

Conversation

behzadmehrabi
Copy link
Contributor

closes #27016.

@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label May 5, 2023
andrewseguin
andrewseguin reviewed May 5, 2023
View reviewed changes
src/material/tooltip/testing/tooltip-harness.ts Outdated
@@ -52,6 +52,11 @@ export abstract class _MatTooltipHarnessBase extends ComponentHarness {
return !!panel && !(await panel.hasClass(this._hiddenClass));
}

/** Gets whether the tooltip is disabled */
async isDisabled(): Promise<boolean> {
return !(await this._optionalPanel());
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be true if the user did not first call show()? Perhaps this method should attempt to show the tooltip, see if it's there, then hide it afterwards in case it wasn't disabled

Copy link
Contributor Author

@behzadmehrabi behzadmehrabi May 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, you're right. 👍

Btw, the reason I didn't call show() inside here is because I saw a method like getTooltipText() requires the user to call show(), otherwise it will return an empty string.(maybe getTooltipText() should also handle show/hide in itself to provide better DX)

crisbeto
crisbeto reviewed May 5, 2023
View reviewed changes
src/material/tooltip/testing/tooltip-harness.ts Outdated
@@ -52,6 +52,11 @@ export abstract class _MatTooltipHarnessBase extends ComponentHarness {
return !!panel && !(await panel.hasClass(this._hiddenClass));
}

/** Gets whether the tooltip is disabled */
async isDisabled(): Promise<boolean> {
return !(await this._optionalPanel());
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't tell you that the tooltip is disabled, it tells you whether it's opened.

Copy link
Contributor Author

@behzadmehrabi behzadmehrabi May 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I took a look at tooltip.ts and that's the only way I could think of to find out about the disabled state.
https://github.com/angular/components/blob/main/src/material/tooltip/tooltip.ts#L420

You are right in a sense that even if the tooltip isn't disabled but has an empty message, this method will return true.
But I think it's kinda correct to return true in that case too. cause at the end, a tooltip with an empty message also acts like a disabled one and doesn't have a different behavior.

Can you think of any other way to handle it?

Copy link
Member

Choose a reason for hiding this comment

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

Usually the way we'd handle it is to add something to the DOM (e.g. a class) that reflects the disabled state.

Copy link
Contributor Author

@behzadmehrabi behzadmehrabi May 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Have a look please.

@behzadmehrabi behzadmehrabi force-pushed the tooltip-harness-is-disabled-method branch from 52214e9 to d3218cf Compare May 5, 2023 13:57
@behzadmehrabi behzadmehrabi force-pushed the tooltip-harness-is-disabled-method branch from d3218cf to 24e204e Compare May 5, 2023 14:25
crisbeto
crisbeto reviewed May 8, 2023
View reviewed changes
src/material/tooltip/tooltip.ts Outdated
@@ -222,8 +225,10 @@ export abstract class _MatTooltipBase<T extends _TooltipComponentBase>
// If tooltip is disabled, hide immediately.
if (this._disabled) {
this.hide(0);
this._elementRef.nativeElement.classList.add(`${this._cssClassPrefix}-${DISABLED_CLASS}`);
Copy link
Member

Choose a reason for hiding this comment

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

This should be done through the host bindings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@behzadmehrabi behzadmehrabi force-pushed the tooltip-harness-is-disabled-method branch from 24e204e to d2c0362 Compare May 8, 2023 06:54
@crisbeto
Copy link
Member

crisbeto commented May 8, 2023

You need to run yarn approve-api tooltip/testing and commit the result to get the api_golden_checks check to pass. The other failure is unrelated.

@behzadmehrabi behzadmehrabi force-pushed the tooltip-harness-is-disabled-method branch from d2c0362 to a22cfaa Compare May 8, 2023 07:26
@behzadmehrabi behzadmehrabi force-pushed the tooltip-harness-is-disabled-method branch 2 times, most recently from e105897 to 705bf60 Compare May 8, 2023 08:47
@behzadmehrabi behzadmehrabi force-pushed the tooltip-harness-is-disabled-method branch from 705bf60 to 09d441e Compare May 8, 2023 09:12
@behzadmehrabi
Copy link
Contributor Author

@crisbeto I ran yarn approve-api tooltip/testing but the api_golden_state was still failed because of legacy-tooltip/testing, so I ran the approve-api for that too.

the integration_tests still failed, am I missing something?

@crisbeto
Copy link
Member

crisbeto commented May 8, 2023

The integration_test failures are unrelated. I'll skip them when merging the PR.

@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker target: minor This PR is targeted for the next minor release labels May 8, 2023
@crisbeto crisbeto self-assigned this May 8, 2023
@angular-robot angular-robot bot merged commit 0028c68 into angular:main May 8, 2023
@behzadmehrabi behzadmehrabi deleted the tooltip-harness-is-disabled-method branch May 9, 2023 06:46
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jun 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@andrewseguin andrewseguin Awaiting requested review from andrewseguin

@crisbeto crisbeto Awaiting requested review from crisbeto

@devversion devversion Awaiting requested review from devversion

@josephperrott josephperrott Awaiting requested review from josephperrott

Assignees

@crisbeto crisbeto

Labels
action: merge The PR is ready for merge by the caretaker detected: feature PR contains a feature commit target: minor This PR is targeted for the next minor release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat(MatTooltipHarness): please add method isDisabled
3 participants
@behzadmehrabi @crisbeto @andrewseguin