Skip to content

Implement comparison methods#14

Merged
3rd-Eden merged 17 commits intogodaddy:masterfrom
SivanMehta:master
Oct 4, 2017
Merged

Implement comparison methods#14
3rd-Eden merged 17 commits intogodaddy:masterfrom
SivanMehta:master

Conversation

@SivanMehta
Copy link
Contributor

@SivanMehta SivanMehta commented Oct 2, 2017

This PR implements the compareHeight and the compareWidth methods. Here is the documentation added to the README that describes the usage of these methods:

compareWidth(breakpoint)

Returns the difference between the current width and the given breakpoint. This can be used to check if the window is "greater" than a breakpoint. If the given breakpoint does not have a width, this will always return the current width. If the given breakpoint does not exist, than we return an error.

var breakpoints = new Breakdancer([
  {
    name: 'wrist',
    width: 320
  },
  {
    name: 'palm',
    width: 800,
    height: 600
  }
]);

// let's assume the window 500 is px wide.
console.log(breakpoints.compareWidth('wrist')) // 180
console.log(breakpoints.compareWidth('palm')) // -300

compareHeight(breakpoint)

Same usage as compareWidth

TL;DR

Now you can check if you're "past" a certain breakpoint with compareWidth('mobile')

Copy link
Member

@Swaagie Swaagie left a comment

Choose a reason for hiding this comment

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

Mostly looking good, seems like a solid feature to add.
Would like to have eyes from @3rd-Eden

test.js Outdated
it('should call compare', function () {
bd.compareHeight('mobile');
bd.compareWidth('mobile');
assume(spy.callCount).equals(2);
Copy link
Member

Choose a reason for hiding this comment

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

might want to assert the functions are called with the right property e.g. width and height

breakdancer.js Outdated

/**
* Compare current and specified properties of given breakpoint
* Requires that this.height() and this.width() are implemented
Copy link
Member

Choose a reason for hiding this comment

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

wonder if we should safeguard against this, although both native and regular have default implementations.

Copy link
Contributor Author

@SivanMehta SivanMehta Oct 3, 2017

Choose a reason for hiding this comment

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

I was thinking that because this is already a @private method, we can safeguard this by relying on the fact this.height() and this.width() methods already exist in index.js and index.native.js.

However, it would be worthwhile to throw a ReferenceError if the associated this[property] function does not exist.

breakdancer.js Outdated
* @param {String} property height or width
* @throws {TypeError} If not given breakpoint and property do not exist within the given spec
* @returns {Number} different between current and specified properties
* @private
Copy link
Contributor

Choose a reason for hiding this comment

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

In all honestly, I would say you might as well just ditch compareHeight and compareWidth and make the compare method public and keep that as only API.

@3rd-Eden 3rd-Eden merged commit 51b7470 into godaddy:master Oct 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants