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

Wrap new has_many items in an <li> tag #3403

Closed
wants to merge 1 commit into from

Conversation

jhanggi
Copy link
Contributor

@jhanggi jhanggi commented Sep 4, 2014

When adding new items in the has_many JS, the new fieldsets are not being wrapped in

  • tags, so the DOM is technically in an invalid state.

  • @coveralls
    Copy link

    Coverage Status

    Coverage remained the same when pulling 1dadb59 on jhanggi:wrap_new_has_many_in_li into 3997f01 on activeadmin:master.

    @seanlinsley
    Copy link
    Contributor

    I assume this is for the situation where f.has_many isn't nested in f.inputs? When it is nested in f.inputs, the HTML structure makes sense:

    screen shot 2014-09-07 at 2 46 20 pm

    @seanlinsley
    Copy link
    Contributor

    I'm going to close this since you never responded. Please feel free to update this PR with additional information so it can be reconsidered.

    @jhanggi
    Copy link
    Contributor Author

    jhanggi commented Oct 21, 2014

    That's fine. I haven't had a chance to verify that it isn't just something specific to how we're using it (we're doing some hackery around the has_many).

    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