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

feat(whiteframe): add directive to apply md-whiteframe classes #6831

Closed

Conversation

devversion
Copy link
Member

@EladBezalel You got assigned for this issue, but everywhere is anyone assigned so I just did that PR. If you had a PR in progress, let me know :)

Fixes #6772

var elevation = attr.mdWhiteframe || DEFAULT_DP;

if (elevation > MAX_DP || elevation < MIN_DP) {
$log.warn('md-whiteframe attribute value is invalid. It should be a number between ' + MIN_DP + ' and ' + MAX_DP, element[0]);
Copy link
Member Author

Choose a reason for hiding this comment

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

This needs to be improved?

@Frank3K
Copy link
Contributor

Frank3K commented Jan 24, 2016

What about the md-whiteframe-zN classes?

@devversion
Copy link
Member Author

That's not said in #6772. But they still can be accessed through the dp number. Need more information from @ThomasBurleson if this is desired.

@Frank3K
Copy link
Contributor

Frank3K commented Jan 24, 2016

I understand DevVersion.

When checking older issues I have found that the -zN system had been deprecated in favor of the dpN system (issues #4706 and #4586).

Maybe it would be a good time to remove the -zN system now. The newly added directive can be introduced at the same time the old -zN class-based system is removed. But that's a decision that should be done by the team.

* @restrict A
*
* @description
* The md-whiteframe directive allows you to apply an elevation shadow to a element.
Copy link
Member

Choose a reason for hiding this comment

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

to an element

@EladBezalel
Copy link
Member

Besides the comments LGTM, @devversion thanks for helping me cleaning my queue :)

@EladBezalel EladBezalel added the needs: review This PR is waiting on review from the team label Jan 24, 2016
@devversion
Copy link
Member Author

@EladBezalel Any time again :) - Applied the suggestions 😄

@@ -0,0 +1,45 @@
div[md-whiteframe] {
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Attribute selectors are slow in IE.
  • This limits the use of md-whiteframe to divs ONLY. Should be md-white-frame

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 thought I should use a div here, as shown in #6772

Copy link
Member

Choose a reason for hiding this comment

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

I don't know how i missed it, you DON'T want to restrict the use to use only div

Copy link
Member Author

Choose a reason for hiding this comment

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

Already updated. Just was confused from the description. Sorry.
PS: This is only the demo, it won't restrict the actual use of the md-whiteframe directive.

@ThomasBurleson ThomasBurleson added needs: work and removed needs: review This PR is waiting on review from the team labels Jan 26, 2016
@ThomasBurleson ThomasBurleson added this to the 1.0.4 milestone Jan 26, 2016
@devversion
Copy link
Member Author

@ThomasBurleson - Added the missing -1 test, updated the breakpoints and annotated them and changed attribute selector to md-whiteframe (as already used here)

ErinCoughlan pushed a commit to ErinCoughlan/material that referenced this pull request Feb 9, 2016
…classes

`md-whiteframe` can now be used as a class or a directive attribute.

Fixes angular#6772, closes angular#6831.
@devversion devversion deleted the feat/whiteframe-directive branch April 21, 2016 12:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants