Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

feat(tab): custom class names for tab elements #11332

Merged
merged 1 commit into from
Sep 4, 2018
Merged

feat(tab): custom class names for tab elements #11332

merged 1 commit into from
Sep 4, 2018

Conversation

benedikt-roth
Copy link
Contributor

This solves #3028.

PR Checklist

Please check that your PR fulfills the following requirements:

  • The commit message follows our guidelines
  • Tests for the changes have been added or this is not a bug fix / enhancement
  • Docs have been added, updated, or were not required

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Enhancement
[x] Documentation content changes
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

We can only select tab-data-item elements via nth- selectors.
This makes solid UI tests difficult.

Issue Number: #3028

What is the new behavior?

You can assign class names via md-class-name attribute
to the md-tab directive. Those will be rendered into the DOM.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@googlebot googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Jun 18, 2018
@Splaktar Splaktar changed the title Custom class names for tab elements feat(tabs): custom class names for tab elements Jun 18, 2018
@Splaktar Splaktar self-assigned this Jun 18, 2018
@Splaktar Splaktar added type: enhancement ui: CSS severity: inconvenient P4: minor Minor issues. May not be fixed without community contributions. labels Jun 18, 2018
@Splaktar Splaktar changed the title feat(tabs): custom class names for tab elements feat(tab): custom class names for tab elements Jun 18, 2018
@Splaktar Splaktar self-requested a review June 18, 2018 23:58
Splaktar
Splaktar suggested changes Jun 19, 2018
View reviewed changes
Copy link
Member

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution!

src/components/tabs/js/tabDirective.js Outdated
@@ -21,6 +21,7 @@
*
* @param {string=} label Optional attribute to specify a simple string as the tab label
* @param {boolean=} ng-disabled If present and expression evaluates to truthy, disabled tab selection.
* @param {string=} md-class-name Optional attribute to specify a class name for the tab button
Copy link
Member

Choose a reason for hiding this comment

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

For consistency with other APIs like md-menu-class, md-menu-container-class, and md-input-class, we should probably call this one md-tab-class.

src/components/tabs/js/tabDirective.js Outdated
@@ -82,7 +83,8 @@ function MdTab () {
active: '=?mdActive',
disabled: '=?ngDisabled',
select: '&?mdOnSelect',
deselect: '&?mdOnDeselect'
deselect: '&?mdOnDeselect',
className: '@mdClassName'
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this optional with @??

@Splaktar Splaktar added this to the 1.1.11 milestone Jun 19, 2018
Splaktar
Splaktar suggested changes Jun 19, 2018
View reviewed changes
Copy link
Member

@Splaktar Splaktar 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 the quick updates! I have a few more minor ones. Also please squash your commits and then force push up your changes to the remote branch.

src/components/tabs/js/tabDirective.js Outdated
@@ -82,7 +83,8 @@ function MdTab () {
active: '=?mdActive',
disabled: '=?ngDisabled',
select: '&?mdOnSelect',
deselect: '&?mdOnDeselect'
deselect: '&?mdOnDeselect',
className: '@?mdTabClass'
Copy link
Member

Choose a reason for hiding this comment

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

Please change className to tabClass.

src/components/tabs/tabs.spec.js Outdated
@@ -450,6 +450,16 @@ describe('<md-tabs>', function () {
expect(tab.find('md-tab-label').text()).toBe('test');
expect(tab.find('md-tab-body').length).toBe(0);
});
it('should render a class name accordingly', function () {
Copy link
Member

Choose a reason for hiding this comment

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

Let's call this should apply tab class on the associated md-tab-item.

src/components/tabs/js/tabDirective.js Outdated
@@ -21,6 +21,7 @@
*
* @param {string=} label Optional attribute to specify a simple string as the tab label
* @param {boolean=} ng-disabled If present and expression evaluates to truthy, disabled tab selection.
* @param {string=} md-tab-class Optional attribute to specify a class name for the tab button
Copy link
Member

Choose a reason for hiding this comment

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

Let's say "Optional attribute to specify a class that will be applied to the tab's button".

@Splaktar Splaktar added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Jun 19, 2018
@benedikt-roth
Copy link
Contributor Author

Thanks for your good feedback. I updated the code accordingly.

Splaktar
Splaktar approved these changes Jun 19, 2018
View reviewed changes
Copy link
Member

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

LGTM

@Splaktar Splaktar added pr: lgtm This PR has been approved by the reviewer and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Jun 19, 2018
@Splaktar Splaktar added the pr: merge ready This PR is ready for a caretaker to review label Jul 8, 2018
@Splaktar Splaktar added the needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved label Aug 20, 2018
@Splaktar
Copy link
Member

@rossgardt can you please rebase?

@benedikt-roth
feat(tabs): Custom class names for tabs
0868100 
`md-class-name` attribute to provide custom
classes for `md-tab`. This is mostly about
more testability of the md-tabs component.

Fixes #3028
@benedikt-roth
Copy link
Contributor Author

@Splaktar done ✅

@Splaktar Splaktar removed pr: presubmit-failures needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved labels Aug 25, 2018
@josephperrott josephperrott merged commit aa30ada into angular:master Sep 4, 2018
nobitagit pushed a commit to nobitagit/material that referenced this pull request Sep 24, 2018
@benedikt-roth @nobitagit
feat(tabs): Custom class names for tabs (angular#11332)
b14a2ec
marosoft pushed a commit to marosoft/material that referenced this pull request Nov 11, 2018
@benedikt-roth @marosoft
feat(tabs): Custom class names for tabs (angular#11332)
763a883
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Splaktar Splaktar Splaktar approved these changes

Assignees

@Splaktar Splaktar

@josephperrott josephperrott

Labels
cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ P4: minor Minor issues. May not be fixed without community contributions. pr: lgtm This PR has been approved by the reviewer pr: merge ready This PR is ready for a caretaker to review severity: inconvenient type: enhancement ui: CSS
Projects
None yet
Milestone
1.1.11
Development

Successfully merging this pull request may close these issues.
6 participants
@benedikt-roth @Splaktar @jelbourn @googlebot @josephperrott @vivian-hu-zz