Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

V6.3 drilldown autoheight #8699

Merged
merged 6 commits into from
Oct 5, 2016

Conversation

designerno1
Copy link
Contributor

Drilldown feature autoheight #8607

this time i used the existing heightcalc, but i think this heightcalc needs a overhaul, because not every child in the menu could have the same height (breaking into 2 lines etc.)

ads features to drilldown
data-auto-adjust-height and data-animate-height
#7821

@kball
Copy link
Contributor

kball commented May 4, 2016

@colin-marshall can you take a look at this and merge if it looks good?

@brettsmason
Copy link
Contributor

From a non-js users point of view this is great. I've just pulled this down and it works brilliantly.
I'll leave it to @colin-marshall to merge though if he is happy with it too!

@colin-marshall colin-marshall self-assigned this May 7, 2016
@emri99
Copy link

emri99 commented May 12, 2016

Confirmed ! It works as expected ;) Thanks @designerno1

agreed, heightcalc need overhaul.
Another problematic use case I experiment currently is using menuitem without text, just an icon. Even in drilldown, I don't want them to be stacked and use float property to align them left or right in one line. heightCalc add a margin-bottom. This is a very specific use case, agreed too ;)

Also wondering the real need of .each block in _getMaxDims method.
Why do you force height when drilldown is closed as the children will define height properly natively ?
I tried to completely comment it without any problems edit: I thought it works due to build delay. This even fix my above use case.
Did I miss anything ?

EDIT:
I think it's only for animation, correct me if I'm wrong: no height or autowill prevent animation.
Height could be set only when animate-height is set, no ?

ie: replace in _hide method

if(this.options.autoAdjustHeight) this.$wrapper.css({height:$elem.parent().closest('ul').data('calcHeight')});

with

if(this.options.autoAdjustHeight) this.$wrapper.css({height: this.options.animateHeight ? $elem.parent().closest('ul').data('calcHeight') : 'auto'});

and add in _getMaxDimsfor initial setup:

if(!this.options.animateHeight) result['height'] = 'auto';

I've test this and the small bottom margin visible before disappear.

I join a patch if you want to take a look ;)
no-animate-height-fix.patch.txt

Thx again

@colin-marshall
Copy link
Contributor

@designerno1 functionality is great! Nice work!

A few observations:

  1. Can we simplify the name of the option to data-auto-height or data-variable-height instead of data-auto-adjust-height?
  2. Defaults are missing for the 2 new options
  3. Docs page should have examples of auto height being on and off.

Other than that I think this is good to go.

@designerno1
Copy link
Contributor Author

@colin-marshall

  1. renamed it
  2. defaults i have missed now added
  3. i added a button with inline onclick JS to the docs to switch autoHeight

later the day i take a look @emri99 patch, and fix the conflicts

@designerno1
Copy link
Contributor Author

@emri99
i remember why i put the heights on autoHeight not to auto with auto the menu got cut of after the initial height on change because of the position style of the element, so i had to set the calced heights to prevent it from get cut of, maybe i can verify that later with a visual test, but thats the reason i remeber

…n-autoheight

# Conflicts:
#	docs/pages/drilldown-menu.md
#	js/foundation.drilldown.js
@rafibomb rafibomb added this to the 6.3 milestone Jul 12, 2016
@kball kball merged commit 563939d into foundation:v6.3 Oct 5, 2016
@kball
Copy link
Contributor

kball commented Oct 5, 2016

This is great! Merged.

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

Successfully merging this pull request may close these issues.

6 participants