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

With current bootstrap4 implementation "vertical" form is impossbile. #573

Closed
shulcsm opened this issue Mar 17, 2016 · 18 comments
Closed

With current bootstrap4 implementation "vertical" form is impossbile. #573

shulcsm opened this issue Mar 17, 2016 · 18 comments

Comments

@shulcsm
Copy link

shulcsm commented Mar 17, 2016

This commit a9477f1 hardcodes classnames that from my perspective are soling edgecase.
There are multiple issues with this:

  1. Broken defaults. Just enabling bootstrap4 templatepack will produce broken forms. Since for horizontal form to work classes for label and field width are expected.
  2. Shoudn't "vertical" form be default? From bootstrap perspective it seems so.
  3. It's impossible to produce "vertical" form at all (you have to override most of the templates)

I'd be happy to fix this, but i'm not very familar with crispy forms. I think the right way to do this would be to add class somewhere conditionally depending on form_style, like its done with form-control?

@carltongibson
Copy link
Collaborator

@shulcsm — feel free to give it a shot!

@zoidbergwill — Can you comment?

@zoidyzoidzoid
Copy link
Contributor

Hi @shulcsm

I'd be happy to help you fix the problem, or fix it myself.

I was worried about the issue when originally working on this, since I was focusing on trying to make it easier for people to migrate a project from Bootstrap 3 to 4.

I'm gonna make sure that I can reproduce what you're talking about, then I'll see if I can fix it.

1.I don't think these forms are broken by default. An inline/horizontal form has never been the default. Bootstrap 4 docs

  1. A "vertical" form looks like the default to me here: image, when I use an example form
  2. By a "vertical" form, do you mean the default Bootstrap form style, or an inline form?

I'm sorry, I'm battling to understand exactly what you mean and reproduce the issue.

It's possible that we just need to make a PR that drops most of the .row classes, like mentioned in my previous commit, and then include in the docs that people will need to add the .row class themselves, where necessary.

If you can reproduce it in my test project and make a PR that adds a failure case, or make a gist that shows the issue, I'd happily help fix it.

@shulcsm
Copy link
Author

shulcsm commented Mar 18, 2016

Sorry for not providing an example. I got your example working, but the issue appears only if you use bootstrap with flexbox enabled. Can't seem to find official build with flexbox enabled.
ekranattels no 2016-03-18 15-29-04

@zoidyzoidzoid
Copy link
Contributor

Wow, that looks terrible.

Thanks. I will have a look at fixing it this evening.

@carltongibson
Copy link
Collaborator

So is this an error in the markup or in Bootstrap?

@zoidyzoidzoid
Copy link
Contributor

I think it's us, because of Flexbox and how it affects .rows, but I'm not actually sure.

I will try a pure Bootstrap example with the changes between Flexbox being enabled and not, tonight.

@carltongibson
Copy link
Collaborator

@zoidbergwill OK. Great. Thanks.

Looking at the example on the bootstrap docs, the row class is not present.

@carltongibson
Copy link
Collaborator

But, of course, in the Using the grid section, there it is...

It looks as if the row class should only be added if the custom widths on label and input are provided.

@zoidbergwill Have a look. See what you think. It might be we need to refactor to support both use-cases...

@zoidyzoidzoid
Copy link
Contributor

Yeah, I added .rows to make migrating from Bootstrap 3 to 4 easier, but now I see it was definitely a mistake to do.

I think that's the issue.

@carltongibson That sounds like a possible solution. Will take a look.

@zoidyzoidzoid
Copy link
Contributor

I've managed to re-create it with https://cask.scotch.io/bootstrap-4.0-flex.css.

Reverting that commit I mentioned fixes the default form, but breaks the horizontal form for flexbox. I'm working on a fix.

@shulcsm
Copy link
Author

shulcsm commented Mar 21, 2016

How about introduction of new form style("horizontal") here https://github.com/maraujop/django-crispy-forms/blob/dev/crispy_forms/helper.py#L254 and using that? That would also make possible adding default column/label width classes.

@zoidyzoidzoid
Copy link
Contributor

Here is what I've done so far with "vertical" and inline/horizontal forms:

Without flexbox:

image

With flexbox:

image

This way adds classes without people having to add them themselves, which is useful.

@shulcsm: That sounds like a good solution, but I think this solution I have might be simpler, if it works, since it still doesn't require users to add a row wrapper_class or anything.

@shulcsm
Copy link
Author

shulcsm commented Mar 21, 2016

Seems good, thanks.

@zoidyzoidzoid
Copy link
Contributor

The alignment of the symbols when using flexbox for the prepdended/appended fields looks weird. Is that expected?

@shulcsm
Copy link
Author

shulcsm commented Mar 23, 2016

That seems to be issue with current alpha, fixed in dev. twbs/bootstrap#19044

@shulcsm
Copy link
Author

shulcsm commented Mar 23, 2016

Are we just waiting for merge? I disovered another issue(not related to flexbox) that this fixes. .row is adding negative margin to fields making them wider than form container.

@carltongibson
Copy link
Collaborator

Yeah. I'm basically happy but want to think it over one more time.

Good news if it fixes other things!

In general I'm expecting the Bootstrap 4 templates to have some issues since they're new and have never been used in action. Any issues you come across: open a ticket.

@zoidyzoidzoid
Copy link
Contributor

I'd love a cleaner solution, but I couldn't think of one. :P

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

No branches or pull requests

3 participants