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

[WIP] Update to Bootstrap 4 #181

Closed
wants to merge 41 commits into from
Closed

Conversation

srvance
Copy link
Contributor

@srvance srvance commented Dec 28, 2016

Update to Bootstrap 4.

Compatibility

General

  • Unbundle less
  • Unbundle bootstrap
  • We need to include an appropriate CSS preprocessor for the dummy app to build and run properly. I don't know if we can have multiple installed. We also should let the user decide between ember-cli-sass and postCSS with SASS. This is a much bigger issue for our testing than it is for the end user.

Features

Feature BS3? BS4? Notes
Accordion + + No change
Alert + + No change
Button + + btn-default changed to btn-secondary. Applied both via computed property since btn-default was handled through type-class. btn-xs dropped; no code change required. The stateful button feature was dropped, but we can retain it.
ButtonGroup + + btn-xs dropped; no code change required.
Collapse + + No change
Dropdown + + Adding dropdown-toggle automatically adds the caret; it does not duplicate if the caret span is present. Add component so that dropdown-item class can be applied for BS4. divider was changed to dropdown-divider; do we want to create a component that's mutually compatible or let people use the one appropriate to their version since it's only a div with a class?
Form + + No change
FormElement + + Just the added class for the partials
FormElement partials + + added classes to checkbox partials
FormGroup + + Implemented in the element component: Add the row class in horizontal layouts. That change still won't work if the form group is used directly.
Modal + + No change. Migration comment about removed functionality should not matter.
Nav + + No change
Nav.item + + Added nav-item class. How should we handle dropdown nav items?
Nav.link-to + + Add component so that nav-link class can be applied for BS4
Navbar + + The icon-bar style was removed, but it's pretty easy to add in. The responsive breakpoint is no longer automatic, so some computed classes need to be added as well as a couple of new classes.
Popover + + No change
ProgressBar BS4 moved to using the progress element instead of a div, but they're reverting back to something similar to the BS3 implementation.
Tab + + Isn't mentioned explicitly in the BS4 docs, including the migration guide, but still works.
Tooltip + + No change. Migration note about removing auto, but I don't think we've implemented it

Open Questions

  • Should we create a dropdown.divider component?
  • How should we handle dropdown Nav items that won't render properly without the nav-item class? We could create a contextual nav.dropdown that would just add some classes. We would need an undocumented inNav parameter to put the nav-item and nav-link classes on the right places.
  • I don't think we should have a special component for Nav links outside of the application. What do you think? I found the link in the Tabs template but could add an explicit nav-link there.
  • How do we handle the test permutations against both BS3 and BS4?
  • How do we unbundle ember-cli-less and still support the dummy app? Dual app styles? Both app.less and app.scss in the dummy app?
  • At what point do we deem BS4 stable enough to incorporate and do we hold up EBS 1.0 for that?

To Do

General

  • Get it to build
  • Get tests passing
  • Update dummy app
  • Assess BS3/BS4 mutual compatibility
  • Write tests for new components
  • Fix the navs type selector in the dummy app
  • Document BS3 to BS4 migration concerns

Features to update

  • Accordion
  • Alert
  • Button
  • ButtonGroup
  • Collapse
  • Dropdown
  • Form
  • FormElement
  • FormElement partials
  • FormGroup
  • Modal
  • Nav
  • Nav.item
  • Navbar
  • Popover
  • ProgressBar
  • Tab
  • Tooltip

and more

Features to add

  • Support form help text
  • Support form grid layout
  • Nav.link-to (for route links)
  • Nav.link? (for non-route links)

@simonihmig
Copy link
Contributor

First of all thanks @srvance for tackling this!

I have not really spend much time thinking about how to add BS4 support. What is your general idea here, it seems you are completely switching to BS4!? As there are probably many apps out there using ember-bootstrap with BS3, I would not want to force them to switch.

So a way of supporting both with the same add-on would be cool. Don't know yet how much effort this would require. Any thoughts?

@srvance
Copy link
Contributor Author

srvance commented Dec 28, 2016

There are a lot of changes. I should have a better assessment later today. It's definitely not a drop-in replacement. Here are some things I have seen so far.

  • There are new classes where there weren't before (e.g., nav-link) but those would be harmless. I think nav-link warrants a new contextual component, but that could easily be compatible.
  • In some cases, class names have changed, but doubling up would also be harmless.
  • Glyphicons have been dropped and may warrant another addon.
  • The full-on SASS usage causes some strange errors if you had less configured, and I don't know if there's a good way to do bootstrap version detection.
  • We might want to unbundle bootstrap if mutual compatibility was a goal, and we would have to double the test permutations.

@srvance
Copy link
Contributor Author

srvance commented Dec 28, 2016

They have a migration doc. It doesn't get into all of the details, but it points you to the right places so far.

@srvance
Copy link
Contributor Author

srvance commented Dec 28, 2016

Most of the changes I'm finding are through visual inspection of the dummy app and weren't caught by the tests. We should talk about whether testing strategy needs to change or completeness increased to ensure compatibility. At least presence of classes needs to be covered more completely, e.g., btn-default/btn-secondary.

@srvance
Copy link
Contributor Author

srvance commented Dec 28, 2016

In the dummy app on the Modals page, unchecking the Open checkboxes results in an "Assertion Failed: Backdrop element should be in DOM" error in the console. Is this expected?

@srvance
Copy link
Contributor Author

srvance commented Dec 28, 2016

Just a little bit left, but I'm not sure I will finish it today.

  • The big issues are around unbundling, testing, and multiple preprocessors.
  • Navbars have some complicated issues.
  • Tabs seem to have disappeared as a first-class entity, but still exist as a nav style; does that make them redundant?
  • ProgressBar isn't working in the dummy app, but I haven't looked closer than that.
  • The use of x-select in the dummy app isn't triggering the variations it's supposed to in a couple spots like Navs. I haven't been able to figure it out yet.

@simonihmig
Copy link
Contributor

Just fixed the modals assertion bug and the broken examples in the dummy app! Pushed to master.

@srvance
Copy link
Contributor Author

srvance commented Dec 29, 2016

Nice. I had fixed a bunch of dummy app stuff, as well.

@srvance
Copy link
Contributor Author

srvance commented Dec 29, 2016

Rebased after the dummy app fixes.

…n the bs-tab template. May need to change when we figure out how to handle dropdowns in navs.
@srvance
Copy link
Contributor Author

srvance commented Dec 29, 2016

@simonihmig I need your input now. Take a look at the Open Questions. Progress bars are going to be problematic because <progress> is more limited than the previous approach.

@srvance
Copy link
Contributor Author

srvance commented Dec 29, 2016

Also, can you take a look at the bs-form/element integration test failure? It seems like it may be too implementation specific but I'm not confident I understand the intent. Should it really just be testing that certain things are aligned together rather than the specifics of their location?

@simonihmig
Copy link
Contributor

I should note that I am in the process of updating the dummy app, to make it the base for the github page (http://kaliber5.github.io/ember-bootstrap/), including an interactive demo section (kind of what the dummy app is currently, but with better examples and code snippets). There will be merge conflicts, so maybe don't spend too much time on the dummy app now!

@simonihmig
Copy link
Contributor

I would not expect BS4 support to land with e-b 1.0. For me the main question is still how/if that should be integrated with the current BS3 based addon:

  • One addon with configurable support for either BS3 or BS4? Like using a config file to record the differences (class names) that the components read from? Would still require different tests. And does not work well when the public API of components would actually have to be different for BS4.
  • a separate addon (like "ember-bootstrap4"), which allows easier implementations if we can statically assume the BS version, but makes updates that both addons should incorporate harder
  • Make a switch to BS4 at a certain point in time (new major version as there would be breaking changes), and only support that from then on

@srvance
Copy link
Contributor Author

srvance commented Dec 30, 2016

Right now, as long as we unbundle ember-cli-less and bootstrap and let the user install the one of their choice, this branch gives complete compatibility with both versions with the exception of progress bars, which will be fixed in BS4 soon. It needs some more validation, but it appears we could go live with a mutually compatible version.

@simonihmig
Copy link
Contributor

What do you mean with unbundling Bootstrap? For the consuming app the default blueprint only adds bootstrap to the app's bower.json (with the right version, which should be changes if BS4 is to be used), and imports (unless deactivated) assets from index.js. Somethings like this should be in place also for BS4 to easily import CSS/SASS.

And ember-cli-less is only used for the dummy app (as a devDependency), we could even switch to plain CSS.

@simonihmig
Copy link
Contributor

And by compatibility you mean the components use the BS4 classes also when used in a BS3 context? I am not sure if that is what we want? Might to lead to undesired side effects?

@srvance
Copy link
Contributor Author

srvance commented Dec 30, 2016

What do you mean with unbundling Bootstrap?

I mean that we either should not include bootstrap with ember-bootstrap and just have them install the bootstrap of their choosing. Changing the bower.json is easy. I don't know if an install option is feasible. The biggest problem becomes the preprocessor.

And ember-cli-less is only used for the dummy app (as a devDependency)

Switching to plain CSS would be better. While ember-cli-less is installed as a devDependency, it still causes conflicts on build to have both competing to create a single css.

And by compatibility you mean the components use the BS4 classes also

Yes, components use extra classes. It's a little dirty, but not too bad a tradeoff for mutual compatibility and simpler implementation. BS4 is better about namespacing than BS3 was, so I see the chance of conflict and undesired side effects to be minimal. I asked mdo if they have a mechanism for version checking, but it doesn't seem they do without scraping comments.

@srvance
Copy link
Contributor Author

srvance commented Dec 30, 2016

We could conditionalize the obsolete and new classes if we could inject a version check of some sort. environment.js config? Can we check the version from bower? We can't really write SASS conditionals for it, since BS3 uses LESS and they could be using plain CSS.

From an API perspective, I introduced new components that we should probably put in 1.0 in preparation even if we don't support BS4 immediately.

@srvance
Copy link
Contributor Author

srvance commented Dec 30, 2016

I should note that I am in the process of updating the dummy app

I'm pretty much done until we decide how to move forward and they release another BS4 alpha to address the progress bar.

@simonihmig simonihmig changed the title Update to Bootstrap 4 [WIP] Update to Bootstrap 4 Dec 31, 2016
@simonihmig simonihmig added the WIP label Dec 31, 2016
@simonihmig
Copy link
Contributor

From an API perspective, I introduced new components that we should probably put in 1.0 in preparation even if we don't support BS4 immediately.

That's the dropdown/item and the nav/link, right? It seems they just add some static classes, so do we really need them? As opposed to just updating the docs? IIRC we had a similar discussion about that with the navbar, whether it makes sense to create components just for static markup? :) I am open to different views though...

What exactly is the issue with the new progress bar?

I will try to put a strategy together how we could integrate BS4 support without dropping BS3, that covers the issues you mentioned (Less vs. Sass, different tests, etc.). But that will have wait for the new year! ;)

You mentioned you were working on a project that required BS4. Is the current state of your fork enough to get you going? I fear that the final, frictionless integration of BS4 might not be possible in the very short-term...

Nevertheless and again thanks a lot for your work so far, really appreciate it! Have a happy New Year! 🎇🍾

@simonihmig
Copy link
Contributor

Hm, this just came to my mind about the nav/link component: currently I reopen the base LinkComponent to mark the active state of the nav's <li> (BS required the active class on the list item, not on the anchor element) according to the active state of the link. This could be dropped if requiring the user to use the special nav/link component. So would probably make sense to integrate this into 1.0-alpha.1...

A bit unsure still about the dropdown/item though. If we add that, then probably also dropdown/divider...

@srvance
Copy link
Contributor Author

srvance commented Dec 31, 2016

re dropdown/item and nav/link: It's just static classes, but it's also what makes it so you have no code changes to switch from BS3 to BS4. A dropdown/divider would do the same thing if it included both BS3 and BS4 classes. This is really a mutual compatibility technique that we wouldn't need if the classes were the same or we were willing to impose some porting requirements. There's a similar open question for nav dropdown items; they don't render properly in BS4 without some additional classes.

What exactly is the issue with the new progress bar?

Their first implementation attempted to use the HTML5 <progress> tag instead, but it's not very cross-browser consistent, has some limits in styling, and removes the min. They just merged the PR to restore the BS3 approach yesterday.

Is the current state of your fork enough to get you going?

It is. I'm using it already. I can't track the head of bootstrap's v4-dev branch because of a bug in bower 1.8.0 so I'm dependent on a release for progress bars and other new developments, but I'm not using them.

@srvance
Copy link
Contributor Author

srvance commented Dec 31, 2016

Have a happy new year! I went a little crazy on this because I'm working on a fun personal project.

@simonihmig
Copy link
Contributor

@srvance pushed to master:

  • bs-form-group as contextual component (Should bs-form-group be contextual? #186)
  • bs-nav/link-to
  • bs-dropdown/menu/item (yielded from bs-dropdown/menu, not bs-dropdown though! I think this allows for more flexibility)

@srvance
Copy link
Contributor Author

srvance commented Jan 9, 2017

FYI, I'm waiting for the next bootstrap alpha release before putting more into this.

@simonihmig Have you thought more about how you want to release this? The idea of maintaining two codebases in parallel just seems like a lot of extra risk and effort compared to a few extra bootstrap classes for the remainder of BS3's life cycle.

@srvance
Copy link
Contributor Author

srvance commented Jan 9, 2017

I just looked and they cut a new release three days ago. It may take me a couple days to get back to it.

@jfrux
Copy link
Contributor

jfrux commented Jan 12, 2017

I may be able to contribute to this too.
I have a project with motivations to push this forward.

@simonihmig simonihmig mentioned this pull request Jan 20, 2017
38 tasks
@simonihmig
Copy link
Contributor

Closing this in favor of #206

@simonihmig simonihmig closed this Jan 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants