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

Add support for fieldAddonLeft / fieldAddonRight for strap selects #34

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TrevorPage
Copy link

This is my first Github pull request and there is at least one issue I've identified with what I've done, so I won't be offended if you reject this first time round.

The original issue is that Bootstrap doesn't let you use the input-group-addon class for selects because apparently they cannot be reliably styled. Therefore ASF does not support the fieldAddonLeft and fieldAddonRight options for selects. But because ASFDS uses buttons, it is possible by modifying the templates. This way, not only text boxes can have these, but selects can, too. Example:

image

I worked on this because I have this use case:

image

I've only updated the templates for the single and multi strap selects so far. I've expanded the example to demonstrate these options.

The problem I've identified is that, because I've had to add the form-control class (which is necessary for Bootstrap to do the addon group styling correctly), all of the selects in the example now occupy 100% width. I can sort of see why this is occurring, but I have not yet spent sufficient time attempting a fix. If this is a tricky issue, one work-around could be to apply the form-control class only if the fieldAddonLeft and fieldAddonRight options are being used.

@nicklasb
Copy link
Member

Yes, selects are surprisingly inflexible.

Really good stuff. I'll take a look later today. WRT to the 100 % width, that could be a problem depending on how the component is used, but I don't know for sure until i have investigated further.

@nicklasb
Copy link
Member

Well, that is strange. It works fine, as far as I can see. But the 100 percent width thing is acting very odd. Strangely, the UI-selects drop downs are resized to fill the area, but the strapselects aren't.
And that doesn't look very nice, in my view, the drop downs should be as wide as the edit field.

I am not that well versed with all the CSS tricks that bootstrap employs, but it seems the standard within bootstrap is a 100 percent width, so the 100 percent might not be a problem.
But the tiny list items, that is another matter.

@nicklasb
Copy link
Member

@TrevorPage : What do you think? I am not sure about this one.

@TrevorPage
Copy link
Author

So just to recap (I'm just thinking this through out loud):

What the above changes have basically done is to steal the <span>s that perform the input-group-addon on the left-hand and right-hand from ASF's text field template, and put those into ASFDS' single- and multi-select strap templates. Furthermore, I've had to add the class form-control to your select <button>, because this is necessary for the input-group-addon classes to work.

The identified side effect is that form-control has the added effect of applying 100% width, causing the select widths in your demo to become wide, screwing the layout. And, more importantly, potentially breaking existing users' layouts.

Now, I have looked at ASF's "kitchen sink" example, and I see that the standard ASF <select>s actually have the form-control class, just as the text controls do. The result is that both selects and text boxes are by default 100% width due to the form-control class.

Therefore, I'd say it's actually more consistent and correct for ASFDS' selects to have form-control. I'm not expert on Bootstrap either unfortunately, but it's entirely possible that all form controls ought to have the form-control class for other reasons (i.e. to do other fancy Boostrappy stuff).

So at the moment I am inclined to suggest that all is correct, and that a single line of custom CSS should be added to the ASFDS demo to remove the 100% width from all form controls, so that the demo layout is fixed. I'm just a little cautious about there being effects I've not thought about.

By the way - I only modified the angular-strap templates so far; the other two just looked scary so I didn't bother changing those yet 😊

@nicklasb
Copy link
Member

Ok, I will look at this. (btw, for some reason I did not get any notifications on this thread)

@nicklasb
Copy link
Member

Indeed, the UI-selects look awful. I have tried several times to clean them up, only to realize that I have to go back to how they work now. It is the UI-selects strange habit of selecting the actual data that is the problem.

@nicklasb
Copy link
Member

Ok, I am not sure that we understand each other, I am talking about that the width of the drop down, the items to select.
I don't mind the control being 100%, but when I run this code, the drop down is just as before, which looks strange. In all the ASF demo page examples, if the control is 100%, the drop down is as well.

@nicklasb nicklasb self-assigned this Aug 19, 2015
@nicklasb
Copy link
Member

I just think it looks wierd when the component is huge and the drop down is tiny:

image

Compared to this, where they are of equal width:
image

@nicklasb
Copy link
Member

@TrevorPage : Do you think that my point is valid?

@nicklasb
Copy link
Member

@TrevorPage : Yo. :-)

@MattiJarvinen
Copy link

I there were some issues in bootstrap with input-group-addon and selects related to webkit. http://getbootstrap.com/components/#input-groups

@TrevorPage
Copy link
Author

Apologies - I have not forgotten this and will ensure I come back and look further.

@nicklasb
Copy link
Member

@MattiJarvinen-BA : Good to know.
@TrevorPage : Looking forward to it!

@TrevorPage
Copy link
Author

I'm finally taking a fresh look at this.

Recap / notes to self

It is the strapselect templates that I modified. The strapselect templates make use of AngularStrap's select capability, which is demonstrated at http://mgcrea.github.io/angular-strap/#/selects. In that demo, we see that:

  • The button element is NOT given the form-control class in their examples, hence normally they are width: auto.
  • The dropdown elements are (in that example) narrower than the button, but they don't look bad, because both parent button and dropdown are all as narrow as they can be.

So, the dropdown being (potentially) narrower is existing behaviour and isn't something introduced by the PR; it's just that the difference becomes especially large when the parent button becomes 100% width due to the addition of form-control. However, the form-control simply has to be added for the left/right fields to work.

One solution

The best solution I can come up with at the moment would be to only apply the form-control class in the template conditionally if left/right fields are in use. This prevents the existing range of selects from being made 100%, so existing applications aren't broken. I've tested this and it works (that is, all the existing selects look like they used to; only the new ones with left/right fields are 100%). We would add a small note to the API documentation informing users that if they use the fieldAddonRight/fieldAddonLeft properties then they must be aware of form-control causing width:100%, which they must manage themselves, perhaps by using simple additional CSS or the grid system.

@TrevorPage
Copy link
Author

If you think this sounds good, I will update the PR with that additional template fix, plus with a sentence added on the example page regarding the 100% width that appears on selects with fieldAddonXXX properties. What's the best sequence of actions to achieve this? My fork is behind by quite a few commits, so should I cancel this PR, then pull latest changes, then do a new PR?

@nicklasb
Copy link
Member

Hi!
Thanks for the explanation!
Ok, I see that now, then the problem is rather that ASF doesn't follow the Bootstrap look.
So yes I am perfectly OK with that solution(with a condition to lessen the impact).

(Yes, please redo the changes against the "develop" branch)

@TrevorPage
Copy link
Author

Oops - for some reason I missed your response; for some reason I don't think I got an email. I will work on redoing the changes against the develop branch as soon as I can. 👍

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.

4 participants