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

Simplify input groups by dropping .input-group-prepend and .input-group-append #29885

Merged
merged 3 commits into from
Mar 23, 2020

Conversation

MartijnCuppens
Copy link
Member

@MartijnCuppens MartijnCuppens commented Dec 19, 2019

_input-groups.scss has become quite hard to maintain, mainly because of the nesting we have with .input-group-prepend and .input-group-append. It's actually pretty simple to remove this. This will also make our html look a lot simpler.

Before:

<div class="input-group mb-3">
  <div class="input-group-prepend">
    <span class="input-group-text" id="basic-addon1">@</span>
  </div>
  <input type="text" class="form-control" placeholder="Username" aria-label="Username" aria-describedby="basic-addon1">
</div>

After:

<div class="input-group mb-3">
  <span class="input-group-text" id="basic-addon1">@</span>
  <input type="text" class="form-control" placeholder="Username" aria-label="Username" aria-describedby="basic-addon1">
</div>

Demo: https://deploy-preview-29885--twbs-bootstrap.netlify.com/docs/4.3/forms/input-group/

Also closes #30111, closes #30104
See https://codepen.io/MartijnCuppens/pen/mdyZBoR

For the dropdown buttons some things needed to be changed in order to make this work because the show class was added to the (random) parent of the toggle button instead to the button itself. The way the dropdown menu is found is now changed.

While I was working on this, I discovered a bug which is explained in #30117. Please review this first because that PR also includes a test.

I've also added an additional example to test multiple dropdowns in a input group: https://deploy-preview-29885--twbs-bootstrap.netlify.com/docs/4.3/forms/input-group/#buttons-with-dropdowns

To Do:

  • Change the way the dropdown menu is detected
  • Change the way active states are set to toggle buttons
  • Adjust existing tests

@alecpl
Copy link
Contributor

alecpl commented Dec 20, 2019

Very nice. Thank you.

@XhmikosR
Copy link
Member

Definitely like this, reduces the final CSS size too 🎉

Copy link
Member

@ysds ysds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some use cases are no longer supported, are they intentional?

v4 supported use cases: #25352

@MartijnCuppens
Copy link
Member Author

@ysds, what use cases exactly?

(I know the input group ↔️ validation message thing is still an issue, but this is not something new?)

@MartijnCuppens
Copy link
Member Author

Ping @ysds

@XhmikosR
Copy link
Member

Just so we are safe, did you check the examples too @MartijnCuppens?

@MartijnCuppens
Copy link
Member Author

@XhmikosR, no appearances of input-group-prepend or input-group-append left. Only the btn-groups are used in the examples.

@ysds
Copy link
Member

ysds commented Jan 31, 2020

@MartijnCuppens Please see dropdown menu's corner in this netlify:

image

That should be rounded.

And cases that have multiple dropdown menus:

image

@MartijnCuppens MartijnCuppens force-pushed the master-mc-drop-input-group-prepend-appends branch from ff247c2 to cd0b3ac Compare January 31, 2020 10:19
@MartijnCuppens
Copy link
Member Author

Just fixed that.

@ysds
Copy link
Member

ysds commented Jan 31, 2020

The 2nd issue (multiple active states with multiple dropdowns) still seems to occur.

@MartijnCuppens MartijnCuppens requested a review from a team as a code owner February 1, 2020 13:56
@MartijnCuppens MartijnCuppens force-pushed the master-mc-drop-input-group-prepend-appends branch 4 times, most recently from 8c82e88 to 137a7fd Compare February 1, 2020 18:10
@ysds
Copy link
Member

ysds commented Feb 10, 2020

The CSS side LGTM. @XhmikosR Could you see the JS side?

@MartijnCuppens
Copy link
Member Author

Please review https://github.com/twbs/bootstrap/pull/30117/files first, it includes a fix and test for the prev() function.

@mdo mdo added the js label Mar 7, 2020
@mdo
Copy link
Member

mdo commented Mar 7, 2020

Punted to Alpha 2 for now, but would happily see it back to first alpha if the JS errors get fixed.

@MartijnCuppens MartijnCuppens force-pushed the master-mc-drop-input-group-prepend-appends branch 2 times, most recently from b64fd1e to 02838f6 Compare March 10, 2020 19:58
@MartijnCuppens
Copy link
Member Author

This fails in IE because of the matches polyfill:

IE 11.0.0 (Windows 10.0.0) Dropdown data-api should fire hide and hidden event without a clickEvent if event type is not click FAILED
	Object doesn't support property or method 'matches' thrown
IE 11.0.0 (Windows 10.0.0) Dropdown data-api should fire hide and hidden event without a clickEvent if event type is not click FAILED
	Object doesn't support property or method 'matches' thrown
................................................
IE 11.0.0 (Windows 10.0.0) Dropdown data-api should ignore keyboard events for <input>s and <textarea>s within dropdown-menu, except for escape key FAILED
	Object doesn't support property or method 'matches' thrown
	Error: Expected true to equal false.
	    at <Jasmine>
	   at Anonymous function (js/tests/unit/js/tests/unit/dropdown.spec.js:1495:9 <- js/tests/unit/dropdown.spec.js:15745:11)
IE 11.0.0 (Windows 10.0.0) Dropdown data-api should ignore keyboard events for <input>s and <textarea>s within dropdown-menu, except for escape key FAILED
	Object doesn't support property or method 'matches' thrown
	Error: Expected true to equal false.
	    at <Jasmine>
	   at Anonymous function (js/tests/unit/js/tests/unit/dropdown.spec.js:1495:9 <- js/tests/unit/dropdown.spec.js:15745:11)

@XhmikosR
Copy link
Member

@MartijnCuppens you can use our helper, or wait until #30377 lands :)

@MartijnCuppens
Copy link
Member Author

The helper is used I guess. I don't really understand the reason why next() is failing and prev() is not

@Johann-S
Copy link
Member

the JS looks good to me, nice work @MartijnCuppens 👍

@MartijnCuppens MartijnCuppens force-pushed the master-mc-drop-input-group-prepend-appends branch from 429c63a to 267ae2a Compare March 23, 2020 13:50
@MartijnCuppens MartijnCuppens force-pushed the master-mc-drop-input-group-prepend-appends branch from 267ae2a to eaeffa1 Compare March 23, 2020 14:04
@MartijnCuppens MartijnCuppens merged commit 2e150e7 into master Mar 23, 2020
@MartijnCuppens MartijnCuppens deleted the master-mc-drop-input-group-prepend-appends branch March 23, 2020 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The style is not strict enough .input-group-prepend .btn+.btn
6 participants