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

Feature/update toggles jquery ui #112

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

kipraske
Copy link
Contributor

This is part of those accessibility improvements that I had mentioned about a month back here https://github.com/andrew-worsfold/tailor/issues/95

I am replacing the internal workings of the toggles element with jquery-ui-accordions which ships with wordpress by default we just need to hook it up. It is heavier than what was there before, but we don't have to worry about all the accessibility things that come with the jquery-ui widgets.

Note I couldn't get the tabs to work properly. I will comment on the issue on where I was in that process. I think our company can just deal with having only toggles and not use tabs until I get around to finishing that part of this.

Things to test:

  • All the options that were on toggles before should all still work the same way as before
  • The "accordion" switch was a bit tricky since that functionality is not really built into jquery-ui so I used some fun internet code. We should be extra critical of that part.

kipraske and others added 11 commits March 9, 2017 09:30
Updating fork to master 3/9/2017
Alright this isn't quite working as we expect. We need a HTML structure
of...

```

accordion-wrapper
 h3
 div
 h3
 div
 h3
 div

```

Here is what we have

```

accordion-wrapper
 toggle-wrapper
  h3
  div
 toggle-wrapper
  h3
  div

```
We just needed to define the header option for the toggles
Turns out I needed to use onInitialize or the built in event listeners
would totally not work. In order to get the backend canvas working we
need those events like onChildChange etc to reinitialize the accordion,
and to get those events we needed to go though the right channels
@andrew-worsfold
Copy link
Contributor

Thanks for these changes, @kipraske! I'll obviously have to test them all thoroughly (and have a think about potential conflicts) before merging, but it looks good at first glance.

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

Successfully merging this pull request may close these issues.

None yet

3 participants