Skip to content
This repository has been archived by the owner on Aug 29, 2023. It is now read-only.

feat(prefixer): add service to prefix attributes #8163

Closed
wants to merge 1 commit into from

Conversation

devversion
Copy link
Member

@devversion devversion commented Apr 21, 2016

The service contains the following features:

  • Check for an element, to have the specified attribute, including the prefixes
  • Generate an attribute selector for the specified attribute
  • Build a list of all prefixed attributes

Fixes #3258. Fixes #8080. Closes #8121

@devversion devversion added the needs: review This PR is waiting on review from the team label Apr 21, 2016
@Frank3K
Copy link
Contributor

Frank3K commented Apr 21, 2016

Thumbs up, this is functionality that is often (or should often be) used.

At first it was not clear to me when one should the the prefixer with an initial parameter or when one should call a method on the instantiated prefixer. When reading the code it became clear to me. I do not have a suggestion yet on how to improve this.

In your PR you add the functionality to a number of components, which is good. However, I think not all components are reached. For example md-autofocus in util.js (references #4180).

$provide.decorator('$mdUtil', ['$delegate', function ($delegate) {

// Inject the prefixer into our original $mdUtil service.
$delegate.prefixer = MdPrefixer;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since most of the examples are prefixing for a querySelector, perhaps this would be better:

$provide.decorator('$mdUtil', ['$delegate', function ($delegate) {
    var prefixer = new MdPrefix();

    // Publish easy alias for 'quick-use'
    $delegate.prefix = angular.bind(prefixer, prefixer.buildList);
    $delegate.prefixForQuery = angular.bind(prefixer, prefixer.buildSelector);

    // Return full prefixer
    $delegate.prefixer = prefixer;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, I don't really like it.

Then the actual prefixer() will be only used for hasAttribute.
This will make the whole constructor function unnecessary.

I would rather change the constructor fn to something like that

$mdUtil.prefixer('ng-click', true)

Copy link
Member Author

Choose a reason for hiding this comment

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

FYI: This comment is already outdated, since @ThomasBurleson and I had a discussion about it and figured out a better solution.

@devversion
Copy link
Member Author

@Frank3K Thanks for the feedback.

Awesome, that the code was enough the clarify the intention. When the code is clear enough to show, how it works, then it's good code (no self-praise 😄)

Yea, I went through the most components and searched for queries, which can be accessed by developers.

Thanks for the alert of the auto-focus directive. I will add some more today, maybe you can help and list some other queries you found ;)

@Frank3K
Copy link
Contributor

Frank3K commented Apr 24, 2016

I have done a quick search but haven't found more.

@devversion
Copy link
Member Author

@Frank3K Great, thanks for your effort. @ThomasBurleson PTAL.

@devversion devversion force-pushed the feat/prefixer-service branch 2 times, most recently from 56ec675 to e3660ba Compare May 27, 2016 15:02
@ThomasBurleson ThomasBurleson added pr: merge ready This PR is ready for a caretaker to review and removed needs: review This PR is waiting on review from the team labels Jun 2, 2016
@ThomasBurleson
Copy link
Contributor

@devversion - merge confict with master. Can you rebase please.

@ThomasBurleson ThomasBurleson added the needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved label Jun 2, 2016
The service is containing the following features:
* Check for an element, to have the specified attribute, including the prefixes
* Generate an attribute selector for the specified attribute
* Build a list of all prefixed attributes

Fixes angular#3258. Fixes angular#8080. Closes angular#8121
@devversion devversion removed the needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved label Jun 2, 2016
@devversion devversion deleted the feat/prefixer-service branch June 11, 2016 15:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr: merge ready This PR is ready for a caretaker to review
Projects
None yet
3 participants