Pull requests

#262 Merged
Repository
zephor zephor
Branch
expander
Repository
atlassian atlassian
Branch
master

New Component: AUI Expander

Author
  1. Benjamin Wong
Reviewers
Description
No description
  • Issues AUI-1269
  • Learn about pull requests

Comments (28)

  1. Jason Berry

    Hrm, I don't particularly like the idea of having something run on DOM ready just to loop over each expander and expand/collapse them as necessary.

    Why not tell them to set the property to show/hide the expanded content on the element directly? Let the delegated event handler capture all the functionality, but the initial state will all be correct.

    Even better, create a Soy template that does this for the developer, who can simply supply the initial state as collapsed or expanded and let the template do the rest. IMO, this component is incomplete without a Soy template.

    1. Benjamin Wong author

      so I think, since we don't really have soy support yet for our plugin devs we can't encourage them to use soy so we still need to polish up the html API (I will still add a soy template though).

      Using that approach how would I get it to hide to a certain height on load? Those calculations require javascript and as far as I could tell having that DOM ready loop was the only way to run javascript on initial load. Would welcome a solution to this if you can think of any clever ways. Basically followed the AUI Tabs approach for this.

      1. Jason Berry

        But we do have Soy support for plugin devs. What do you think I've been using for JIRA's Bamboo plugin? :)

        Need to have a think about the certain height thing.

        I could tell the AUI Tabs approach was used for this, which is why I was like "noooooooooooo". There is nothing in tabs.js that needs to run on DOM ready… NOTHING. (well, except for the vertical tabs text clipping detection, but other than that, nothing :P)

  2. Jason Berry

    The tests/demo page should be a soy template, not a flat html file which involves copy/pasting and maintaing huge chunks of untemplated markup. We're not doing that any more! See all the other recently created components…

  3. Sean Curtis

    It doesn't work well if you've increased the size of the font (Cmd +). I'm with Jason - I'd prefer not to have a FOUEC (Flash Of Un-Expanded Content) on page load. It won't manifest itself in such a simple test page but once you get that code into a product page with a lot more markup/js it'll be noticeable.

    1. Ben Buchanan

      How would you solve it? It's the X-lines calculation that's the real headache.

      We could set this up so you could apply CSS prior to the JS running; ie. have a class that's .ihavenotbeeninitialised ... So you could set a height with css:

      .scope .ihavenotbeeninitialised { height: somevalue; }
      

      ...and have the expander remove the class when the JS kicks in. Could still get a flicker, of course. For the options that don't have that uncertainty, just having expanded/collapsed as an initial state will probably do.

      1. Jason Berry

        The x-lines is really just min-height, right? So use the min-height CSS property to actually do what it does, then read out min-height in the JS when the trigger is activated?

            1. Benjamin Wong author

              so instead of having an API to set the height, the developer using the component has to manually set min-height for the component to work. Is that what you're saying?

              1. Benjamin Wong author

                Or ignore setup if min-height exists, and only use the data property to hide it? (This will potentially cause differing initial height and hide-height if you change one but not the other)

                1. Jason Berry

                  Well they have to set a data-aui-expander-height property anyway, might as well use min-height instead (and height: 0; if it's collapsed, set height to default height when expanded) and not have to worry about doing all the shit in JS.

                  Does that make sense?

                  1. Adam Ahmed

                    +1, moving the initialization JS into some form of CSS is a must. The AJS.tabs.setup() crap causes no end of headache and I'd prefer we never used that pattern again.

                    Jason's solution sounds like the right way to go, IMO. You can even put the min-height into the Soy as an inline style if necessary.

                    1. Benjamin Wong author

                      done, downside is the multi-line thing no longer works but I guess you can always use js to calculate the min-height in the implementation so that's ok

  4. Sean Curtis

    Also it'd be handy to add a link to the expander to the base test page index file so those of us that check out the repo can get to the test page easily.

      1. Benjamin Wong author

        I will, in my rush to commit I left it in there because there's still examples I want to pull in. I figured it can live there for now until I get time to clean it up properly.

  5. Benjamin Wong author

    discussed elsewhere, will remove the high level soy template as it is not essential to core functionality. Implementations wil have to call trigger and content separately for now.