[WIP] Use rails-bootstrap-forms gem for form rendering

[soullivaneuh]

This PR is related to #8470.

The goal is to use rails-bootstrap-form gem to simplify form rendering on bootstrap.

To do:

• Install rails-bootstrap-form gem
• Rewrite all form rendering with bootstrap-form helpers (admin section only for the moment, next with another pull requests)
• Wait for btn-group forms management: Possible to style checkboxes / radios as buttons? bootstrap-ruby/bootstrap_form#132
• Wait "HTML help block" issue answer: HTML code on help section bootstrap-ruby/bootstrap_form#211. Concern those forms:
  • https://github.com/gitlabhq/gitlabhq/blob/master/app/views/admin/applications/_form.html.haml#L17-L21
• Wait "Static control without label" issue answer: Static control without label bootstrap-ruby/bootstrap_form#212. Concern those forms:
  • https://github.com/gitlabhq/gitlabhq/blob/master/app/views/admin/broadcast_messages/index.html.haml#L14-L20
• Wait "Unwanted space with hide_label option on inline form" answer: Unwanted space with hide_label option on inline form bootstrap-ruby/bootstrap_form#214. Concern those forms:
  • https://github.com/gitlabhq/gitlabhq/blob/master/app/views/admin/groups/show.html.haml#L61-L67
  • https://github.com/gitlabhq/gitlabhq/blob/master/app/views/admin/users/index.html.haml#L24-L28
• Wait "Unwanted hidden input on check_box helper with bootstrap_form_tag" answer: Unwanted hidden input on check_box helper with bootstrap_form_tag bootstrap-ruby/bootstrap_form#215. Concern those forms:
  • https://github.com/gitlabhq/gitlabhq/blob/master/app/views/admin/projects/index.html.haml#L6-L24
• Wait "Ability to create radio button without label?" answer: Ability to create radio button without label? bootstrap-ruby/bootstrap_form#161. Concern those forms:
  • https://github.com/gitlabhq/gitlabhq/blob/master/app/views/admin/services/_form.html.haml
• Fix weird css issue: [WIP] Use rails-bootstrap-forms gem for form rendering #9055 (comment)
• Fix bad visibility form rendering on application settings: [WIP] Use rails-bootstrap-forms gem for form rendering #9055 (comment)
• Add some notes on UI best practices documentation [form_for / form_tag / alert_messages / f.primary]
• All tests should be green
• Update CHANGELOG file

Pending questions:

• Should we remove .form-fieldset class and add padding by default on fieldset element?
• Is the new form error rendering correct?
• Can Rubocop have custom rules detection? Could be useful to have a form_for usage detection, isn't it? (make bootstrap_form_for required. Use text_field_without_bootstrap is necessary)
• Should I use static control field for special subforms? ([WIP] Use rails-bootstrap-forms gem for form rendering #9055 (comment))
  • No, use form_group instead.
• OK for using required label css trick? ([WIP] Use rails-bootstrap-forms gem for form rendering #9055 (comment))

Externals links:

• https://github.com/bootstrap-ruby/rails-bootstrap-forms

Please tell me if I have some other task to do on this subject.

Thanks.

[soullivaneuh]

I saw that sometime .form-fieldset is used just to add some margin on fieldset.

Code: https://github.com/gitlabhq/gitlabhq/blob/master/app/assets/stylesheets/generic/forms.scss#L84-L86

Why not put this margin by default on fieldset?

[soullivaneuh]

Can't convert btn-group to bootstrap-form at the moment because it's not managed.

Example here: https://github.com/gitlabhq/gitlabhq/blob/master/app/views/admin/application_settings/_form.html.haml#L45-L52

Issue opened, waiting for news: bootstrap-ruby/bootstrap_form#132

[soullivaneuh]

Got weird form rendering on validation error, like this:

Have to be fixed.

[soullivaneuh]

Invisible fields and label on error is due to this overridden variable used by .has-error text color.

But If I disable it, alert text-color are not white anymore.

Any suggestion?

[soullivaneuh]

Fixed. Now we have this:

Sounds good for you?

Will be the same on all form. Maybe we can store error message with config or constant. What is you suggestion about that?

[soullivaneuh]

Can Rubocop have custom rules detection? Could be useful to have a form_for usage detection, isn't it?

[dzaporozhets]

The goal is to use rails-bootstrap-form gem to simplify form rendering on bootstrap.

Looks awesome. code looks clean

[soullivaneuh]

Yes @randx I think so too.

The less code you have to do, the happier developer you will be.

A good developer is a lazy developer, remember that! ;) 👍

[soullivaneuh]

Just made group forms.

Have to use f.static_control for avatar and alerts but it renders quite differently:

Not the same padding on avatar form because of <p> static control, and alerts are quite separated.

What do you think? Don't know for the moment if I can do better with this lib. Any suggestion?

[dzaporozhets]

I am not sure I understand :)

[soullivaneuh]

What did you not understand @randx? Surely by mad english... ;)

[soullivaneuh]

Find a way to fix that. Use form_group instead of static_control for sub forms.

[soullivaneuh]

Got some UI issue on inline form without label.

See related created issue: bootstrap-ruby/bootstrap_form#214

[dzaporozhets]

@soullivaneuh cool. I didnt understand what padding you talked about. But does not matter - glad you managed to fix it

[soullivaneuh]

Got some strange html render with checkbox tag (not form_for).

Issue created: bootstrap-ruby/bootstrap_form#215

[soullivaneuh]

Just updated admin projects filters but got strange results:

Could be custom css fault?

[soullivaneuh]

Just updated admin service templates.

Was this before:

And after:

Seems good for you?

Please not the double "Active" label. On issue is pending about that here: bootstrap-ruby/bootstrap_form#161

[soullivaneuh]

Just updated the admin users form.

I also taken profit to introduce the required label css tricks: https://github.com/bootstrap-ruby/rails-bootstrap-forms#required-fields

This will add a * on every required form.

So we turn the old manual * required text like this:

To this:

More cleaner isn't it? 👍

More cooler, this will be automatically applied on every required field of each form using bootstrap_form_(tag|form).

Are you OK with this?

You can see the related edited css here: https://github.com/gitlabhq/gitlabhq/pull/9055/files#diff-8173fb057f92d7588e1800c179fe111cR46
And a field example here: https://github.com/gitlabhq/gitlabhq/pull/9055/files#diff-583833dd037fc513229f947d3467c520R7

Regards.

[dzaporozhets]

Cool!

[dzaporozhets]

@soullivaneuh i am a bit afraid that PR will be hard to complete and review since it touches every single form in application. Maybe we can do it step by step converting several forms per PR?

[jvanbaarsen]

@soullivaneuh I like the small tweaks this PR brings to the UI :) Great job so far!

[soullivaneuh]

@jvanbaarsen Thanks! 😄

The admin section is completely rewritten but some rails-bootstrap-forms issues still have to be resolved before merge this PR.

So I'm blocked for continuing.

@randx Maybe another solution. Keep continue the integration of another sections on this PR but with one commit by section. With this method, you could see and review the change section by section just by clicking to the concerned commit.

If you find this interesting, I can setup a demo of my PR for easier reviewing.

What do you think?

Regards.

[dzaporozhets]

Keep continue the integration of another sections on this PR but with one commit by section

@soullivaneuh I want to merge reviewed parts so I propose you just create another PR on top of this branch with next section. When I merge this one - it will be only uniq changes per section in next PR

If you find this interesting, I can setup a demo of my PR for easier reviewing.

thank you. I think its not needed - I will use localhost to check

[dzaporozhets]

So try to fix tests here and prepare it for review before starting new section

[soullivaneuh]

@randx

So try to fix tests here and prepare it for review before starting new section

Yes will do that.

After this, I will separate section by subPRs directly on my form. With that, you could review it easily.

After review, I'll just have to merge all my subPR and then, this one.

This method will be easier for me if I have to modify the main branch.

Are you OK for that?

[soullivaneuh]

I can't run test on my local, got the following error:

  Scenario: I change my code preview theme
    ✔  Given I sign in as a user                                       # features/steps/shared/authentication.rb:7
    ✔  Given I visit profile design page                               # features/steps/shared/paths.rb:126
    ✔  When I change my code preview theme                             # features/steps/profile/profile.rb:121
    ✔  Then I should receive feedback that the changes were saved      # features/steps/profile/profile.rb:132
/home/sullivan/.rvm/gems/ruby-2.1.3/gems/activerecord-4.1.9/lib/active_record/connection_adapters/postgresql/database_statements.rb:128:in `exec': PG::Error: ERROR:  deadlock detected (ActiveRecord::StatementInvalid)
DETAIL:  Process 22429 waits for AccessExclusiveLock on relation 17755 of database 16385; blocked by process 22447.
Process 22447 waits for AccessShareLock on relation 17562 of database 16385; blocked by process 22429.
HINT:  See server log for query details.
: TRUNCATE TABLE "application_settings", "broadcast_messages", "deploy_keys_projects", "emails", "events", "forked_project_links", "identities", "issues", "keys", "label_links", "labels", "members", "merge_request_diffs", "merge_requests", "milestones", "namespaces", "notes", "oauth_access_grants", "oauth_access_tokens", "oauth_applications", "project_import_data", "projects", "protected_branches", "services", "snippets", "subscriptions", "taggings", "tags", "users", "users_star_projects", "web_hooks" RESTART IDENTITY CASCADE;

Any idea?

[rumpelsepp]

Run it again. :)
Currently it's a bit buggy. I struggled with the same problem a few days ago: #9184 (comment)

[jvanbaarsen]

@soullivaneuh Are you still working on this?

[soullivaneuh]

@jvanbaarsen Yes.

Sorry I have not lot of time at this moment (apartment search). I think I will go back actively on this PR in two weeks maximum.

Regards.

[maxlazio]

Good luck with your search!

[rspeicher]

Personally I'd prefer simple_form using its Bootstrap-specific initializer. That way we're less tightly coupled to Bootstrap for the future.

[soullivaneuh]

simple_form seems to be less complete. For sample, I don't see on the doc how to set an horizontal or vertical form.

BTW I already changed a lot of template. I would prefer to keep it as is unless other collaborators wants to change too.

[jvanbaarsen]

@soullivaneuh I believe @randx agreed on this approach, so I think you can continue this way :)

[rspeicher]

For sample, I don't see on the doc how to set an horizontal or vertical form.

It's part of the Bootstrap initializer: https://github.com/plataformatec/simple_form/blob/master/lib/generators/simple_form/templates/config/initializers/simple_form_bootstrap.rb

[jvanbaarsen]

@randx What do you think? I would propose to continue the way @soullivaneuh has already been taken. The argument from @tsigo "That way we're less tightly coupled to Bootstrap for the future." is not really valid, since when we would change from bootstrap, we would have way bigger problems then just our form library (like our entire HTML markup).

[dzaporozhets]

@jvanbaarsen I agree with you. First, I can not imagine GitLab without bootstrap. And seconds @soullivaneuh already did quite big amount of work already. It would be waste if we loose this progress. I am absolutely ok with rails-bootstrap-forms as soon as it nicely implemented without breaking UI on any page

[soullivaneuh]

Ok so I continue on this way. 👍

[soullivaneuh]

Weird rendering on application settings form:

[dblessing]

Hi @soullivaneuh Do you have time to come back to this?

[soullivaneuh]

@dblessing I nearly finish admin section. But vendor bugs are still here.

Maybe it would be better to juste let bootstrap-form implementation, configuration and documentation and continue integration on another PRs.

[soullivaneuh]

ping @dblessing

[jvanbaarsen]

This merge request has been closed because a request for more information has not been reacted to for more than 2 weeks. If you respond and conform to the merge request guidelines in our contributing guidelines we will reopen this merge request.

[soullivaneuh]

@jvanbaarsen AFAIK, I'm still waiting an answer of @dblessing.