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

feat(collapse): add possibility to collapse horizontally #6010

Closed
wants to merge 7 commits into from

Conversation

squelix
Copy link
Contributor

@squelix squelix commented Jun 15, 2016

New attribute to collapse horizontally or vertically : put 'horizontal' to collapse horizontally or put nothing to do it vertically

Plunkr : http://plnkr.co/edit/EXr6cBJ58wfRRDlvCl5i?p=preview

Fixes #3593

New directive to collapse horizontally or vertically : put 'horizontal' to collapse horizontally or pu nothing to do vertically
@squelix squelix changed the title feat(directive): add possibility to collapse horizontally feat(collapse): add possibility to collapse horizontally Jun 15, 2016
squelix added 2 commits June 15, 2016 14:57
Correct the horizontal collapse example
Forget to detect the 'horizontal' attribute
easing: 'ease',
to: { height: element[0].scrollHeight + 'px' }
}).start()['finally'](expandDone);
if (horizontal) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It probably would be worthwhile converting everything in here into a helper function to avoid duplication between horizontal and vertical collapsing.

@wesleycho
Copy link
Contributor

This mostly looks good, just needs some code cleanup to avoid unnecessary repetition.

@squelix
Copy link
Contributor Author

squelix commented Jun 15, 2016

Sorry for two of my commit.
I made a lot of code cleanup.

Pluckr updated : http://plnkr.co/edit/EXr6cBJ58wfRRDlvCl5i?p=preview

@icfantv
Copy link
Contributor

icfantv commented Jun 15, 2016

We have an open issue (#3593) for this. Thank you for taking the initiative to do this...

horizontal = !!('horizontal' in attrs);
if (horizontal) {
cssWidth = {width: 'auto'};
cssHeight = {height: 'inherit'};
Copy link
Contributor

Choose a reason for hiding this comment

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

Height and width can be combined into one object, i.e.

css = {
  width: 'auto',
  height: 'inherit'
};

@wesleycho
Copy link
Contributor

A lot of minor issues, clean those up and this should be good to go

@wesleycho wesleycho added this to the 2.0.0 milestone Jun 15, 2016
Refactor to had one call of css() and remove spaces
@squelix
Copy link
Contributor Author

squelix commented Jun 16, 2016

@icfantv I needed this feature so i made it and i sent it here :)

cssTo = {width: '0'};
} else {
cssWidth = {width: 'inherit'};
cssHeight = {height: 'auto'};
css = {
Copy link
Contributor

Choose a reason for hiding this comment

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

the same CSS is in both if blocks - that can be combined.

Copy link
Contributor

@wesleycho wesleycho Jun 16, 2016

Choose a reason for hiding this comment

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

This is different CSS - note the inversion of order of values

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So is this good ? I can't combine because that's not the same value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, it's fine

Copy link
Contributor

Choose a reason for hiding this comment

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

gah! sorry. i totally missed that.

@wesleycho
Copy link
Contributor

I think this is good for merge - what do you think @icfantv?

@icfantv
Copy link
Contributor

icfantv commented Jun 16, 2016

Yep, doooo eeeeeet.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants