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

Expanded default item #12

Closed
ain opened this issue Feb 11, 2016 · 8 comments
Closed

Expanded default item #12

ain opened this issue Feb 11, 2016 · 8 comments
Assignees

Comments

@ain
Copy link
Contributor

ain commented Feb 11, 2016

Feature request: user would like to have one accordion item expanded by default as the page has loaded.

@ain
Copy link
Contributor Author

ain commented Feb 11, 2016

To deliver the feature, I'd propose using data-accordion-expanded attribute on item(s) with a boolean value.

@GeorgMeyer23
Copy link

👍

@davleh
Copy link
Contributor

davleh commented Feb 23, 2016

Has been already implemented, just add the defined panelActiveClass to the panel that should be open initially.

@davleh davleh closed this as completed Feb 23, 2016
@ain
Copy link
Contributor Author

ain commented Feb 23, 2016

I think since we're dealing with a component, we should think in component space as opposed to representation space. API bindings over data attributes would IMO be more appropriate and how it's done in most cases.

panelActiveClass as such would be something that defines how it works internally, but it's not as readable in terms of external behaviour/initialisation. Also for the backend implementations and developers.

@davleh
Copy link
Contributor

davleh commented Feb 23, 2016

What is more easy than setting a class?

@ain
Copy link
Contributor Author

ain commented Feb 23, 2016

Setting a class is not easy. It involves CSS and JS. CSS should be used for representation not function. If no class is given, component should still remain functional. Setting a requirement on a class setting couples function with presentation. So it is not easy, and it is prone to error.

@davleh
Copy link
Contributor

davleh commented Feb 23, 2016

You can set it when outputting the markup, so in the template. I do not see the problem here. The whole component is build upon classes, they are needed for the accordion to work. It has to find its panels and the connected content and trigger classes, they also have to be set, so while iterating over the accordion panel item data which comes from backend it should be easy to set an additional active class in the frontend.

@ain
Copy link
Contributor Author

ain commented Mar 1, 2016

Class-based API is generally a bad idea. Internally, organised classes make a nice way to do it, but revealing an API over classes is not going to work. Here's why:

  1. In most cases you need events dispatched from the component when something happens, e.g. a node is opened. With only classes, it's a no-can-do.

  2. It's error-prone. As said, a class should represent presentation, it's good for that. But it can't describe functionality and become a method as API endpoint. This is what happens to this component when only class is used to initially expand the 1st item:

    screen shot 2016-02-29 at 18 40 43

  3. It's not DRY, e.g. by a realistic code example:

%ul{data: {component: 'iptAccordion', component-options" => '{"panelActiveClass": "expandable__list__item--active"}'
  - list_items.each |list_item, index|
    %li{ :class => ("expandable__list__item--active" if index == 0) }

For these reasons I'll reopen the ticket for feature development as originally planned.

@ain ain reopened this Mar 1, 2016
@ain ain added the enhancement label Mar 1, 2016
@ain ain self-assigned this Mar 1, 2016
ain added a commit that referenced this issue Mar 1, 2016
In reference to #12 Expanded default item
@davleh davleh closed this as completed in 084abb7 Mar 1, 2016
davleh pushed a commit that referenced this issue Mar 1, 2016
…nd-over-data-attribute-12

Resolve #12 Expanded default item
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants