Skip to content

feat(material-experimental): add test harness for button #16556

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

Merged
merged 1 commit into from
Jul 26, 2019

Conversation

jelbourn
Copy link
Member

No description provided.

@jelbourn jelbourn added pr: merge safe target: patch This PR is targeted for the next patch release labels Jul 17, 2019
@jelbourn jelbourn requested a review from andrewseguin as a code owner July 17, 2019 20:06
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 17, 2019
<button id="fab" type="button" mat-fab>Fab button</button>
<button id="mini-fab" type="button" mat-mini-fab>Mini Fab button</button>

<a id="anchor-basic" type="button" mat-button>Basic anchor</a>
Copy link
Member

Choose a reason for hiding this comment

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

Links don't have a type attribute.

* @dynamic
*/
export class MatButtonHarness extends ComponentHarness {
static hostSelector = [
Copy link
Member

Choose a reason for hiding this comment

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

There's an array of these defined as BUTTON_HOST_ATTRIBUTES. Could we export and re-use it here? Otherwise we'll have to remember to update this array if we add more button types.

Copy link
Member Author

Choose a reason for hiding this comment

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

I really just want to change this to use e.g. .mat-button-base, which was in another PR when I wrote this. Added a TODO for now

}

/** Gets a promise for the button's name. */
async getName(): Promise<string | null> {
Copy link
Member

Choose a reason for hiding this comment

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

Is getting the name of a button common enough for it to have it's own method? We could expose a getAttribute instead which would cover more cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed. I was on the fence myself, having just left it in there from checkbox

* @dynamic
*/
export class MatButtonHarness extends ComponentHarness {
static hostSelector = [
Copy link
Member

Choose a reason for hiding this comment

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

Same as the comment above importing these

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a TODO

Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

LGTM aside from @crisbeto's comments. One question though: why do we put in the standard button harness into the mdc-button entry-point?

Copy link
Member Author

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

Comments addressed, added code to skip the disabled click test for Edge

* @dynamic
*/
export class MatButtonHarness extends ComponentHarness {
static hostSelector = [
Copy link
Member Author

Choose a reason for hiding this comment

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

I really just want to change this to use e.g. .mat-button-base, which was in another PR when I wrote this. Added a TODO for now

}

/** Gets a promise for the button's name. */
async getName(): Promise<string | null> {
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed. I was on the fence myself, having just left it in there from checkbox

* @dynamic
*/
export class MatButtonHarness extends ComponentHarness {
static hostSelector = [
Copy link
Member Author

Choose a reason for hiding this comment

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

Added a TODO

@jelbourn jelbourn force-pushed the button-harness branch 2 times, most recently from 33f61b0 to c8414a7 Compare July 23, 2019 22:54
Copy link
Contributor

@mmalerba mmalerba left a comment

Choose a reason for hiding this comment

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

LGTM asside from comment nits

/**
* Gets a `HarnessPredicate` that can be used to search for a button with specific attributes.
* @param options Options for narrowing the search:
* - `label` finds a button with specific label text.
Copy link
Contributor

Choose a reason for hiding this comment

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

update comment

/**
* Gets a `HarnessPredicate` that can be used to search for a button with specific attributes.
* @param options Options for narrowing the search:
* - `label` finds a button with specific label text.
Copy link
Contributor

Choose a reason for hiding this comment

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

update comment

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Jul 26, 2019
@jelbourn jelbourn merged commit 88631b9 into angular:master Jul 26, 2019
@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 Sep 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants