-
Notifications
You must be signed in to change notification settings - Fork 284
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
Bootstrap v4.1 support #56
Conversation
Thank you so much for working on this. There are too many changes in the same PR. Can you open others PRs to upgrading the Rails version and other unrelated changes to bootstrap and keep this one only focused in adding bootstrap 4 support? |
* add User model with validations * add custom excluision_array_validator * add i18n
* generated by Balsamiq wireframe * export all as png
* js helper to pre-fill forms with data * js helper for toggle, tooltip, and swap
* `form_abbr` * `form_collection_label` * `form_custom_checkbox_switch` * `form_floating_labels` * `form_invalid` * `form_multi_select`
# Conflicts: # app/controllers/examples/base_controller.rb
* `date_fields_test` * `floating_labels_test` * `form_helper_test` * `submit_test`
* vertical forms * horizontal forms * inline forms * custom fields form * floating labels form * input groups forms
* re-use bootstrap input-padding defaults
* add error classes to inputs * load lastest `simple_form` version from github * remove custom form_invalid styles :link: heartcombo/simple_form#1552
* refine inline form markup * use `span` wrapper * use shorter `:error` instead of `:full_error`
Gemfile
Outdated
@@ -67,7 +67,8 @@ gem 'bootstrap', '~> 4.0.0' | |||
gem 'jquery-rails' | |||
gem 'kramdown', '~> 1.16' | |||
gem 'rouge', '~> 3.1' | |||
gem 'simple_form', '~> 3.5' | |||
gem 'simple_form', github: 'plataformatec/simple_form' | |||
gem 'validates_timeliness', '~> 4.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need this gem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just wanted easy and readable validations for the date
, time
and datetime
examples. But open for better suggestions.
validates :birthday, presence: true, timeliness: { type: :date, :after => lambda { Date.today } }
validates :awake, presence: true, timeliness: { type: :time, before: '12:00' }
validates :first_kiss, presence: true, timeliness: { type: :datetime, after: '20:00' }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. Thank you for explaining.
module Components | ||
module InputGroups | ||
module Prepends | ||
def prepend(wrapper_options = nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to upstream to to simple_form so we can remove it from here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, great! What do you suggest. Should I open a new PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, please open an PR to simple_form or an issue so we can discuss the solutions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done - heartcombo/simple_form#1554
* apply API to register custom components * heartcombo/simple_form#1556
simple form now 💜 bootstrap 4.1 The bootstrap generator is extracted from: heartcombo/simple_form-bootstrap#56 * supported form wrapper all covered by *tests* * vertical form * horizontal form * inline form * custom fields form * input groups form * floating labels form :link: heartcombo/simple_form-bootstrap#56 🔗 heartcombo#1561 Resolves heartcombo#1337
kudos @m5o ! |
simple form now 💜 bootstrap 4.1 The bootstrap generator is extracted from: heartcombo/simple_form-bootstrap#56 * supported form wrapper all covered by *tests* * vertical form * horizontal form * inline form * custom fields form * input groups form * floating labels form :link: heartcombo/simple_form-bootstrap#56 🔗 #1561 Resolves #1337
* update simple_form to v4.0.0 * https://rubygems.org/gems/simple_form
* update rails to v5.2.0 * http://railsdiff.org/5.2.0.rc2/5.2.0
* fixes issue with Incompatible units: 'px' and 'rem'.
|
* update bootstrap gem to v4.1 * `bundle update bootstrap`
@rafaelfranca So I'm finally done 🙌 Rails I have added a Travis CI setup, would you do the setup or alternatively give me the permission to the demo repo 🤓 Happy merging 🚀 |
# Use this setup block to configure all options available in SimpleForm. | ||
SimpleForm.setup do |config| | ||
# Default class for buttons | ||
config.button_class = 'btn' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be 'btn btn-primary'
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only if you want all buttons have btn-primary
style. You could run intro override troubles then.
If you want to use variations like btn-outline-*
Look at this example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern though is that the default behavior of f.button :submit
(i.e. without specifying a class) produces a button which disappears on mouse hover:
Checking through the Bootstrap 4 docs, it's not clear to me whether btn
is meant to be used by itself. It seems like it's always supposed to be paired with a btn-*
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's supposed to be paired with an additional class. But I understand your concern, with c617d81 I've added a style modifier that target elements with only a .btn
class.
You can use this to avoid specifying a class to your f.button :submit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea. It might even be nice addition to Bootstrap itself! For Simple Form though, should it be scoped to .simple_form [class="btn"]
to prevent contamination?
Also, I'm not sure, but do you know if .btn[class="btn"]
would have better selector performance than just [class="btn"]
? I know that attribute selectors are slower than class selectors, and I know that hierarchical selectors (e.g. .a .b .c
) are evaluated right-to-left, but I don't know if browsers are smart enough to use the .btn
part first, then filter on [class="btn"]
(i.e. evaluate that single selector left-to-right).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Try to make a PR to bootstrap.
* apply primary button style to buttons with only `btn` class
Bootstrap is now 4.1.1 |
* update bootstrap to v4.1.1 * `bundle update bootstrap`
* tweak styles to match bootstrap v4.1.1
b.optional :readonly | ||
b.use :label, class: 'form-control-label' | ||
b.wrapper tag: 'div', class: 'd-flex flex-row justify-content-between align-items-center' do |ba| | ||
ba.use :input, class: 'custom-select mx-1', error_class: 'is-invalid', valid_class: 'is-valid' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of having mx-1
here? In a newly-generated form it throws off alignment.
In the demo there is an override which resets the margins back to zero:
.custom-select[multiple], .custom-select:only-child {
margin-left: 0 !important;
margin-right: 0 !important;
}
But I can't tell where that override is supposed to come from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mx-1
adds horizontal margin to the inputs. Without this extra padding multiple custom selects looking glued bad together. Please send a screenshot/demo of your alignment problem.
The style overrides are optional, they are documented and have to be added manually to perfectly match bootstrap output. These additional styles are suggested. See documentation page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a screenshot of what I am referring to:
The select doesn't adhere to the left and right edges of the column, which is particularly noticeable in the middle of a bunch of text inputs. (The custom date and other custom_multi_select
inputs share the same problem.)
I see now the custom styling in the documentation that you were referring to. I have just been following the SImple Form readme, so it wasn't clear to me that additional steps were needed. What do you think about including the additional stylesheets in the Simple Forms railtie so that they can be imported directly from the simple_form gem (without the need for copying and pasting so many files)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's outside my responsibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jonathanhefner I too have been having a confusing time figuring out the right way to get things to look right with the default generated simple_form botostrap config intiializer.
I support the idea making a PR to have such a stylesheet, if you want to do it, give it a try and see if the maintainers will approve?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jrochkind I submitted #60 which attempts to solve the issue using only the config initializer, but the PR seems to be stuck in limbo. If you want to copy "config/initializers/simple_form_bootstrap.rb" from that branch into your project though, that should be all you need.
For a preview of how it looks, see the screenshots in the companion PR for the main repo: heartcombo/simple_form#1587.
# <%= error_messages_for(@object) %> | ||
# | ||
# Markup aliged with bootstrap v4 | ||
module ErrorMessagesHelper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this? This was something that Rails had support for it and we removed because it was too magical, I really don't want to add it back in an example application.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's currently unused in the example application. I thought it would be a good idea to add it as a reference. You never know, if some people find this useful. It's from a working example of a side project of mine. That's why I add comments and kudos to ryanb/nifty-generators. Of course will remove it, if it bothers you.
Imho, Simple Forms build in f.error_notification
has a very minimalistic and simple output. The ErrorMessagesHelper
/ f.error_messages
-helper with Bootstrap v4 markup would just provide a reference for some more extended error messages. Ready to use out-of-the-box. Example attached.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fwiw, I think supporting both options is most sensible for this library, since not everyone using it must necessarily keep using inline errors and it also helps when migrating custom non-simple_form elements' styling.
Amazing, thank you so much @rafaelfranca & @m5o |
All praises to @m5o, I only pressed the button 😄 |
🏁 Final 3 of 3
Hey @rafaelfranca 👋
simple form now 💜 bootstrap 4.1
📺 Demo preview
Info
fieldset
/legend
for multiple check boxes / radio buttonsI've tested a heroku deployment by doing:
heroku create
git push heroku bootstrap-v4:master
heroku run rake db:migrate
For updating the existing heroku app, please drop the DB.
heroku pg:info
heroku pg:reset
heroku run rake db:migrate
/cc @Nerian @carlosantoniodasilva @feliperenan
🔗 heartcombo/simple_form#1337