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

Floating labels don't work inside input groups (prepend/append) #642

Open
joeroe opened this issue May 23, 2022 · 16 comments
Open

Floating labels don't work inside input groups (prepend/append) #642

joeroe opened this issue May 23, 2022 · 16 comments

Comments

@joeroe
Copy link

joeroe commented May 23, 2022

Floating labels don't display correctly if they're used with an input group, i.e. if prepend: or append: are set.

f.text_field :name, floating: true, prepend: "My name is"

Generates:

<div class="mb-3 form-floating">
    <div class="input-group">
        <span class="input-group-text">My name is</span>
        <input class="form-control" placeholder="Name" type="text" value="" name="person[name]" id="person_name">
    </div>
    <label class="form-label" for="person_name">Name</label>
</div>

To work, I think there should only be one wrapper element, and the label should be placed alongside the input, i.e.:

<div class="mb-3 form-floating input-group">
    <span class="input-group-text">My name is</span>
    <input class="form-control" placeholder="Name" type="text" value="" name="person[name]" id="person_name">
    <label class="form-label" for="person_name">Name</label>
</div>
@lcreid
Copy link
Contributor

lcreid commented Jul 2, 2022

@UweKubosch @thimo This is a really interesting find. It makes me think that we've put the input-group class in the wrong place up until now. I'm going to put together a PR that I believe will fix this, but I suspect it's going to change a lot of test cases. If you have a chance, take a look at this issue and what it would take to fix it. Maybe you'll find something better than what I'm going to do.

@thimo
Copy link
Collaborator

thimo commented Jul 5, 2022

input-group only has 41 occurrences in the project, of which 24 in 2 specs and 12 in the README. So I think it's not going to be too much of a hassle to change. Please let me know if there's something that I can do to help.

@lcreid
Copy link
Contributor

lcreid commented Jul 6, 2022

I wonder about changing the HTML. As I read it, we need to move the input-group to the <div class='mb-3'> that's more or less the Bootstrap 5 equivalent of the Bootstrap 4 form-group. Do you think we would cause too much pain if we change the HTML? I think it's justifiable that we're fixing a bug and sorry if you relied on our mark-up, but I'm interested in what others think.

At the very least I realized we should document more clearly that we reserve the right to change the HTML when we realize that we've made a mistake in interpreting how we're supposed to use Bootstrap.

@thimo
Copy link
Collaborator

thimo commented Jul 6, 2022

Ah yes, didn't consider possibly breaking existing apps. Can we include this as a "breaking change" in the next minor release, probably 5.2? Then document why and what the impact is. I would think we at most would break a bit of CSS, but that's a dangerous assumption. Still I think we should fix this.

@lcreid
Copy link
Contributor

lcreid commented Jul 7, 2022

Yes, I agree that's it's actually "broken" and needs to be fixed. I guess that's the nature of CSS that when we fix something, someone might have written some CSS that won't work after our fix. But so be it. Thanks for your input!

@shivam091
Copy link

Hello,

Is it going to be changed accordigly to support input groups inside floating labels ?

Thanks

@lcreid
Copy link
Contributor

lcreid commented Aug 2, 2022

Yes, @shivam091 it's being worked on. It requires some non-trivial changes to the way the code is organized internally. If you would like to try putting together a PR to fix this, let us know. Otherwise, we appreciate your patience.

@shivam091
Copy link

I have tried it in my way by cloning the gem.
Few changes I have done are as follows which gives me desired output but It's achived by editing entire structure of the code which you added for rendering the field.

Let's take example for password_field

def form_group(*args, &block)
  options = args.extract_options!
  attribute_name = args.first

  form_group_classes = form_group_classes(options)

  label = generate_label(options[:id], attribute_name, options[:label], options[:label_col], options[:layout], options[:floating])
  help_text = generate_help(attribute_name, options[:help_text])

  options[:class] = form_group_classes if form_group_classes.present?

  if group_layout_horizontal?(options[:layout])
    prepend_and_append_input(attribute_name, options) do
      concat(label) << tag.div(capture(&block) + help_text, class: form_group_control_class(options))
    end
  elsif options[:floating]
    tag.dl(**form_group_attrs(options)) do
      concat(prepend_and_append_input(attribute_name, options) do
        concat(tag.div(class: "form-floating") do
          concat(capture(&block))
          concat(label)
        end)
      end)
      concat(help_text)
    end
  else
    tag.dl(**form_group_attrs(options)) do
      concat(label)
      concat(prepend_and_append_input(attribute_name, options) do
        capture(&block)
      end)
      concat(help_text)
    end
  end
end
def password_field(attribute_name, options = {})
  label_options = options.fetch(:label, {})
  form_group_builder(attribute_name, options) do
    options[:placeholder] ||= label_text(attribute_name, label_options) if options.delete(:floating)
    super(attribute_name, options)
  end
end
= form.password_field :password, data: {secret_reveal_target: "input"}, floating: true, input_group: {append: password_reveal_button}

Removed unused form_group_content(label, help_text, options, &block) from form_group.rb

Refer SS:
Screenshot from 2022-08-03 07-15-33
Screenshot from 2022-08-03 07-15-53

@shivam091
Copy link

The above are major tweaks I have added in for getting the output but it might have some other breaks which I need to check and fix @lcreid

@lcreid
Copy link
Contributor

lcreid commented Aug 3, 2022

Thanks for this. You're in the right area all right. If you'd like to submit a pull request, I can review it. Check out our Contributing document. I have made some progress but I won't be able to finish anything before the weekend.

@shivam091
Copy link

Yes I will try. I am not having idea on writing test cases therfore I may also require some amount of time. I try my best to submit the PR.

Thank you

@lcreid
Copy link
Contributor

lcreid commented Aug 8, 2022

Thanks for giving this a go, @shivam091 If you're struggling to get all the tests working, I have something that I think is working. The existing code didn't make this change easy. If you want to continue with the PR, I suggest that you create a draft with just the tests, so we can agree on what the new HTML should look like.

@shivam091
Copy link

Yes @lcreid, I check it and try to submit tests asap.

@shivam091
Copy link

<div class="input-group">
  <span class="input-group-text">@</span>
  <div class="form-floating ">
    <input class="form-control" placeholder="Password" type="password" name="user[password]" id="user_password">
    <label class="required" for="user_password">Password</label>
  </div>
  <span class="input-group-text">@</span>
</div>

This markup is generated with my changes which I thing perfext according to Bootstrap 5.2 upgrades.

NOTE - I considered upgrading bootstrap to 5.2 because styles required for input groups and floating labels for a element was missing in 5.0 5.1 versions.

@lcreid
Copy link
Contributor

lcreid commented Aug 10, 2022

I think that HTML will work. However, be sure to try it with the prepend and append options, too.

This JSFiddle shows the difference between the OP's solution and yours: https://jsfiddle.net/qhem392r/4/.

Nice find about the versions. We'll may have to change the installation instructions so people use the right version of Bootstrap.

@donv
Copy link
Collaborator

donv commented Sep 15, 2023

I am taking a look at this issue now.

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

5 participants