Update to bootstrap@4.0.0-beta
#410
Conversation
|
Hey @srvance, thanks for tackling this! I am just wondering if looking at the demo app is the best way to approach this. I think it is still to coupled to BS3, as it is just meant to drive the docs site. Given that, I think that we might get too many "false positives" when looking at it for visual issues? Shouldn't we just compare the rendered output of the (BS4) component and compare that to the markup the BS docs site suggests, and see if there is some mismatch? |
|
I've had good results in the past running the demo app with So far, I'm wondering if the docs have completely caught up with the implementation and how much of the implementation was moved into the js. For example, see the first bullet here and note that there is a lot of raw styling appearing now when you toggle a dropdown in the BS docs. Anyway, this may take awhile. The changes are significant. I'm just starting the investigation. Any findings at this point should be taken as tentative. 😄 |
|
@simonihmig It appears that things like dropups, popovers, and tooltips are now being handled by Popper.js, which injects styling directly onto the element. There's an ember-popper, still in alpha, that we could use for the same effect. It seems BS4 has delegated that part of the styling, though, only using Popper to manage it. I filed twbs/bootstrap#23399 to see if I'm missing something. |
|
@srvance What kind of styling do you see managed by popper.js? I just had a look, and it seems to me this is just limited to positioning things. So e.g. for popovers and tooltips this are similar CSS props like |
|
Definitely positioning styling, but also something complicated to replicate. What do you think of bringing in ember-popper to bridge the gap and helping to bring it to release? Or would you rather just bring popper.js in directly? |
|
Accordion seems to be wrong, the checklist says there are no changes but This is true for a lot of class names across the system, it's pretty significant from my knowledge from alpha6. |
|
Thanks for the sharp eye, @joshuairl! Let me know if you see anything else. |
|
Fixes #421 |
|
@simonihmig Two things:
|
| } | ||
| }), | ||
|
|
||
| appliedType: 'light', |
There was a problem hiding this comment.
Why is this separate property needed?
There was a problem hiding this comment.
type is used by the TypeClass mixin and I needed somewhere to store the value while giving it a default value. I got tired of wonder why the hamburger menu didn't show when running the demo app in BS4. It's bitten me three times already. It's because it sets type to undefined if the checkbox is unchecked. Perhaps I shouldn't override the use of default, though. What do you think?
There was a problem hiding this comment.
I might be missing something, but can't you just return a literal 'light' in the CP's getter, and remove the this.set call in the setter?
There was a problem hiding this comment.
That was my first approach, but I needed the setter to redirect the set anyway. I'll look at it again and see if it can be reduced or simplified.
Sorry, but which comment are you referring to? Might be missing something...
Yeah, sorry for the late feedback on that topic... I think that is acceptable. popper.js itself and the ember addon seem solid, tested, small. The addon also supports older Ember versions (even 1.11+), popper is tested for all browsers, including IE10+ (don't know about IE9). Some questions though:
|
Yeah, I was a little vague. In the todo list above, the comment about spacing next to the "Forms" sub-item. |
|
re popper, let me see if we can use it for both and reduce the code. |
|
Towards inline form spacing: that's hard with our current API. I wouldn't like to have to introduce some very explicit properties like I would tend to "postpone" this for now. I don't know but I actually never used inline forms, and if you do need them, you can still implement them with your own custom markup, nobody forces you to use those What do you think? Regarding the popper thing, If you want me to help or take this over (like exploring using that as the replacement for our existing BS3-based custom positioning), just say so. You should not think that you are obliged to work on all this upcoming BS4 stuff exclusively! 😉 |
|
I'm fine deferring the spacing. Inline forms are mostly what's used in the demo app for the meta-controls, which is where this becomes apparent. We should probably just add some spacing there if we want to support it in BS4. I won't have much time to look into popper until next weekend. Feel free to look into it if you want. That will cover a large amount of what's left. How do you feel about a compatible API change for the dropdown toggle? |
|
hio, i don't think it is possible at the moment (bs4-beta branch) to use a from https://getbootstrap.com/docs/4.0/components/navbar/
|
|
Not sure if this is being tracked and I just didn't see an item for it, but BS4 significantly changes how form validation errors are displayed. For instance, And on an ease-of-use note, it would be awesome if errors added to a model (eg |
|
Other than the test failure, no. The remaining checklist items can be follow on. I don't think the dropdown toggle is an API change. |
|
I'm getting errors on this branch when I hover over an item that has a tooltip. I suspect it's related to ember-popper changes? |
|
@simonihmig I'm finding my time is more limited than anticipated right now. Unless you see a showstopper, let's track outstanding issues separately and merge this. I'll let you make the final call on the right timing for this, but I think it's ready for broader use. |
|
@srvance no problem at all! Agree, will merge this soon. There will be some more @bgentry can you please elaborate, what errors exactly do you see? (there was an ember-popper issue with IE11 fixed recently, but I guess that's not what you are referring to?) |
|
@simonihmig just tried this again, looks like another ember-popper update fixed the errors (e06ee9a) 👍 |
|
@bgentry Oh, that's great! Thanks for the super quick feedback!!! |
|
🎉 ❗️ |
Updating to BS4 beta, all tests pass.
The following issues are observed in the components in the demo app:
card-blocktocard-bodyember-power-selectissue and unrelated to BS4)showclass moved to the menu)navbar-toggleable-*tonavbar-expand-*bs-navbarhandlesnullorundefinedvalues as its defaults, so that has been fixed.)Typedropdown (This seems to be anember-power-selectissue)Additional:
base