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

feat(layouts): convert layout attribute selectors to class selectors #4282

Closed
wants to merge 2 commits into from

Conversation

paulflo150
Copy link
Contributor

Created directives to add CSS classes to elements which appears to help IE's selector engine.

Fixes #1771. Closes #4282.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@paulflo150
Copy link
Contributor Author

I signed it!

Updated layout.scss with hide/ show class selectors.
@ThomasBurleson
Copy link
Contributor

@jelbourn, @robertmesserle, @rschmukler - can you guys make a quick review of this?

These changes appear to work great in Chrome, Safari, and FireFox...
I imagine the IE performance will show significant performance improvements.

I proposed the packaging of src/core/services/layout/ since we have JS files.

@paulflo150 - great work here. fantastic turnaround. love the tests.
@timnix - thx for your help and input.

};
}

for (var i = 0; i < mappings.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move this for loop block to line 32... so the function definitions will subsequently follow.
The testWithSuffixAndValue() logic is also difficult to understand and maintain.

Try this:

function testWithSuffixAndValue(attribute, values, suffix, addDirectiveAsClass) {

  for (var j = 0; j < values.length; j++) {
    var value = values[j].toString();
    var attr = suffix ? attribute + '-' + suffix : attribute;

    var attrWithValue = buildAttributeWithValue(attr, value);
    var expectedClass = buildExpectedClass(attr, value);

    // Run each test.
    testMapping(attrWithValue, expectedClass);
  }

  /**
   * Build string of expected classes that should be added to the 
   * DOM element.
   *
   * Convert directive with value to classes
   *
   * @param attrClass String full attribute name; eg 'layout-gt-lg'
   * @param attrValue String HTML directive; eg "column"
   * 
   * @returns String to be used with element.addClass(...); eg  `layout-gt-lg layout-gt-lg-column`
   */
  function buildExpectedClass(attrClass, attrValue) {
    if (addDirectiveAsClass) attrClass += ' ' + attrClass;
    return $mdUtil.supplant('{0}-{1}', [attrClass, attrValue.replace(/\s+/g, "-")]);
  }

  /**
   * Build full string of expected directive with its value
   * Note: The expected class always starts with the
   *     attribute name, add the suffix if any.
   *
   * @param attrClass String full attribute name; eg 'layout-gt-lg'
   * @param attrValue String HTML directive; eg "column"
   * 
   * @returns String like `layout-gt-lg="column"`
   */
  function buildAttributeWithValue(attr, value) {
    return $mdUtil.supplant('{0}="{1}"', [attr, value]);
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

You will need to inject the $mdUtil service:

inject(function($mdUtil) {
  for (var i = 0; i < mappings.length; i++) {
     var map = mappings[i];
     ...
  }
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is what I was looking at, but the test will not start with the inject wrapper, so I must be missing something:

describe('layouts service', function () {

    beforeEach(module('material.layouts'));

    describe('expecting layout classes', function () {

        var suffixes = ['sm', 'gt-sm', 'md', 'gt-md', 'lg', 'gt-lg'];

        var directionValues = ['row', 'column'];
        var flexValues = [0, 5, 10, 15, 20, 25, 30, 35, 40, 45, 50, 55, 60, 65, 70, 75, 80, 85, 90, 95, 100, 33, 34, 66, 67];
        var alignmentValues = ['center', 'center center', 'center start', 'center end',
                           'end', 'end-center', 'end start', 'end end',
                           'space-around', 'space-around center', 'space-around start', 'space-around end',
                           'space-between', 'space-between center', 'space-between start', 'space-between end',
                           'center center', 'start center', 'end center', 'space-between center', 'space-around center',
                           'center start', 'start start', 'end start', 'space-between start', 'space-around start',
                           'center end', 'start end', 'end end', 'space-between end', 'space-around end'];
        var flexOrderValues = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9];
        var offsetValues = [5, 10, 15, 20, 25, 30, 35, 40, 45, 50, 55, 60, 65, 70, 75, 80, 85, 90, 95, 33, 34, 66, 67];

        var mappings = [
                { attribute: 'layout', suffixes: suffixes, values: directionValues, addDirectiveAsClass: true, testStandAlone: true },
                { attribute: 'layout-align', suffixes: suffixes, values: alignmentValues },
                { attribute: 'layout-padding', testStandAlone: true },
                { attribute: 'layout-margin', testStandAlone: true },
                { attribute: 'layout-wrap', testStandAlone: true },
                { attribute: 'layout-fill', testStandAlone: true },

                { attribute: 'flex', suffixes: suffixes, values: flexValues, addDirectiveAsClass: true, testStandAlone: true },
                { attribute: 'flex-order', suffixes: suffixes, values: flexOrderValues },

                { attribute: 'offset', suffixes: suffixes, values: offsetValues },

                { attribute: 'hide', suffixes: suffixes, testStandAlone: true },
                { attribute: 'show', suffixes: suffixes, testStandAlone: true }
        ];

        inject(function ($mdUtil) {
            for (var i = 0; i < mappings.length; i++) {
                var mapping = mappings[i];
                // First test the mapping without any suffixes or values.
                if (mapping.testStandAlone)
                    testMapping(mapping.attribute, mapping.attribute);
                // Check for suffixes.
                if (mapping.suffixes)
                    testWithSuffix(mapping.attribute, mapping.suffixes, mapping.values, mapping.testStandAlone, mapping.addDirectiveAsClass);
                // Check for values.
                if (mapping.values)
                    testWithSuffixAndValue(mapping.attribute, mapping.values, undefined, mapping.addDirectiveAsClass);
            }
        });

        function testMapping(attribute, expectedClass) {
            it('should fail if the class ' + expectedClass + ' was not added for attribute ' + attribute, inject(function ($compile, $rootScope) {
                var element = $compile('<div ' + attribute + '>Layout</div>')($rootScope);
                expect(element.hasClass(expectedClass)).toBe(true);
            }));
        }

        function testWithSuffix(attribute, suffixes, values, testStandAlone, addDirectiveAsClass) {
            for (var j = 0; j < suffixes.length; j++) {
                var suffix = suffixes[j];
                // Add the suffix to the attribute.
                var attributeWithValue = attribute + '-' + suffix;
                // Add the suffix to the expected class.
                var expectedClass = attribute + '-' + suffix;
                // Run the test.
                if (testStandAlone)
                    testMapping(attributeWithValue, expectedClass);
                // Add suffixes with values.
                if (values)
                    testWithSuffixAndValue(attribute, values, suffix, addDirectiveAsClass);
            };
        }

        function testWithSuffixAndValue(attribute, values, suffix, addDirectiveAsClass) {

            for (var j = 0; j < values.length; j++) {
                var value = values[j].toString();
                var attr = suffix ? attribute + '-' + suffix : attribute;

                var attrWithValue = buildAttributeWithValue(attr, value);
                var expectedClass = buildExpectedClass(attr, value);

                // Run each test.
                testMapping(attrWithValue, expectedClass);
            }

            /**
             * Build string of expected classes that should be added to the 
             * DOM element.
             *
             * Convert directive with value to classes
             *
             * @param attrClass String full attribute name; eg 'layout-gt-lg'
             * @param attrValue String HTML directive; eg "column"
             * 
             * @returns String to be used with element.addClass(...); eg  `layout-gt-lg layout-gt-lg-column`
             */
            function buildExpectedClass(attrClass, attrValue) {
                if (addDirectiveAsClass) attrClass += ' ' + attrClass;
                return $mdUtil.supplant('{0}-{1}', [attrClass, attrValue.replace(/\s+/g, "-")]);
                //return attrClass + '-' + attrValue.replace(/\s+/g, "-");
            }

            /**
             * Build full string of expected directive with its value
             * Note: The expected class always starts with the
             *     attribute name, add the suffix if any.
             *
             * @param attrClass String full attribute name; eg 'layout-gt-lg'
             * @param attrValue String HTML directive; eg "column"
             * 
             * @returns String like `layout-gt-lg="column"`
             */
            function buildAttributeWithValue(attr, value) {
                return $mdUtil.supplant('{0}="{1}"', [attr, value]);
                //return attr + '="' + value + '"';
            }
        }
    });
});

@ThomasBurleson ThomasBurleson changed the title performance enhancement IE (layout) feat(layouts): create layout class selectors for IE performance enhancement Aug 22, 2015

'use strict';

angular.module('material.layouts', ['material.core'])
Copy link
Contributor

Choose a reason for hiding this comment

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

Add this comment header to line 4:

/**
 *
 *   The original ngMaterial Layout solution used attribute selectors and CSS.
 *  
 *  ```html
 *  <div layout="column"> My Content </div>
 *  ```
 *  
 *  ```css
 *  [layout] {
 *    box-sizing: border-box;
 *    display:flex;  
 *  }
 *  [layout=column] {
 *    flex-direction : column
 *  }
 *  ```
 *  
 *  Use of attribute selectors creates significant performance impacts in some 
 *  browsers... mainly IE. 
 *  
 *  This module registers directives that allow the same layout attributes to be 
 *  interpreted and converted to class selectors. The directive will add equivalent classes to each element that 
 *  contains a Layout directive.
 * 
 * ```html
 *   <div layout="column" class="layout layout-column"> My Content </div>
 *```
 *  
 *  ```css
 *  .layout {
 *    box-sizing: border-box;
 *    display:flex;  
 *  }
 *  .layout-column {
 *    flex-direction : column
 *  }
 *  ```
 */

@ThomasBurleson ThomasBurleson changed the title feat(layouts): create layout class selectors for IE performance enhancement feat(layouts): convert layout attribute selectors to class selectors Aug 22, 2015
@ThomasBurleson
Copy link
Contributor

@paulflo150 - While testing this PR locally, I unintentionally, prematurely committed this PR to master... before the team review was completed. As such - to ensure all tests and code run correctly - I modified the source [in master] with the commit SHA 46b5500.

Please base future PRs for material.core.layout off of the updated master branch.

And thanks again for an excellent initial PR.

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.

3 participants