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

[WIP] Animated Accordion API #2517

Closed
wants to merge 2 commits into from
Closed

Conversation

moaxaca
Copy link

@moaxaca moaxaca commented Feb 11, 2018

Hey guys,

I've made this in response to issue: #2114

Disclaimer: This isn't completed I want to talk to the maintainers on API design. I haven't included test coverage but I will when the API design is solidified.

I am opening up this PR to promote a conversation on how to expose decorated animated interfaces. I was looking for examples of modules being decorated by Transitions and couldn't find a convention.

I've included with this PR two ways of accomplishing the behavior one from a new top layer decorator of the Accordion -> AccordionAnimated. Secondly, I've also included a more intrusive example where I define the animation props against the AccordionContent. I don't expose the full transition API in the current implementation and would like to discuss a strategy on doing that gracefully.

Looking forward to some feedback when you guys get the chance!

Also kudos on the codebase this being my first exposure it was extremely easy to bootstrap and contribute.

@welcome
Copy link

welcome bot commented Feb 11, 2018

💖 Thanks for opening this pull request! 💖

Here is a list of things that will help get it across the finish line:

  • Run yarn lint locally to catch formatting errors. This will fix some errors automatically, commit and push any changes.
  • Run yarn test locally to catch errors. This ensures all components still behave as they should.
  • Run yarn start to run the doc site locally and try a few pages, ensuring everything is in good working order.
  • Include tests when adding/changing behavior.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@brianespinosa brianespinosa changed the title Animated Accordion API [WIP] Animated Accordion API Feb 12, 2018
@brianespinosa
Copy link
Member

Thanks for putting this together @moaxaca. I think we will want some input from @layershifter on this since I believe he had some vision on how we could reduce boilerplate when he created the Transition component. Ideally we could have one decorator component that could be reused on other components that is not specific to Accordion. So the AccordionAnimated component would be something more generic. I'll let him chime in here in case I am recalling his comments incorrectly from when he was taking his second stab at adding a Transition component.

@moaxaca
Copy link
Author

moaxaca commented Feb 13, 2018

@brianespinosa
That sounds good, I am looking forward to hearing from him. One that I discovered while implementing this the lack of CSS3 transitions against auto-calculated values. Here is a document describing the problem and some workarounds. I should note this only applies to the container of the accordion, not the content that's being animated.

https://css-tricks.com/using-css-transitions-auto-dimensions/

I will explore the current implementation of Transition and see if there's a graceful solution.

@GCrispino
Copy link
Contributor

Good to know that there’s someone working on this. I’m new to the Semantic UI codebase, but I’m interested in contributing to this enhancement, if needed

@moaxaca
Copy link
Author

moaxaca commented Feb 19, 2018

@GCrispino Thanks, were currently waiting for the maintainers to engage.

@GCrispino
Copy link
Contributor

@moaxaca Ok cool!

@tayupov
Copy link

tayupov commented Mar 14, 2018

What's the status on this?

@moaxaca
Copy link
Author

moaxaca commented Mar 16, 2018

@tayupov need author feedback. @brianespinosa could we get the label "needs author feedback"

@Whipgit
Copy link

Whipgit commented Apr 5, 2018

How can we get a follow-up to this?

@layershifter
Copy link
Member

The right way was described for the implementation there.

@moaxaca
Copy link
Author

moaxaca commented Apr 12, 2018

@layershifter I will review and take a look at it this weekend.

@stale
Copy link

stale bot commented Jul 11, 2018

There has been no activity in this thread for 90 days. While we care about every issue and we’d love to see this fixed, the core team’s time is limited so we have to focus our attention on the issues that are most pressing. Therefore, we will likely not be able to get to this one.

However, PRs for this issue will of course be accepted and welcome!

If there is no more activity in the next 90 days, this issue will be closed automatically for housekeeping. To prevent this, simply leave a reply here. Thanks!

@stale stale bot added the stale label Jul 11, 2018
@moaxaca moaxaca closed this Jul 11, 2018
@hugomn
Copy link

hugomn commented Jul 29, 2018

@moaxaca Why was this closed? Can I help somehow?

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.

7 participants