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

Remove bootstrap #285

Merged
merged 4 commits into from
Jan 2, 2020
Merged

Remove bootstrap #285

merged 4 commits into from
Jan 2, 2020

Conversation

bunnymatic
Copy link
Contributor

@bunnymatic bunnymatic commented Dec 11, 2019

Problem

Bootstrap, though it can help to get things rolling, it is not ideal in
the modern day where we often move rapidly to a Javascript
library/framework that has CSS-in-JS or some other scheme to manage
styling.

Including bootstrap includes (implicitly) it's own reset CSS that is
fine at first. But if you later decide to remove Bootstrap, you may
find that your components no longer look right because you've lost that
implicit reset.

Solution

Remove bootstrap

Changes

  • Remove bootstrap from package.json
  • update the stylesheets we have to be a bit simpler but still rendering
    a header and footer

Screenshots (optional):

Main screenshot

Screen Shot 2019-12-11 at 11 50 08 AM

Imagine we have a model called Thing with attribute name, here are the scaffolded templates.

Screen Shot 2019-12-11 at 11 50 35 AM
Screen Shot 2019-12-11 at 11 50 45 AM
Screen Shot 2019-12-11 at 11 50 51 AM

@bunnymatic bunnymatic changed the title Remove bootstrap DRAFT : Remove bootstrap Dec 11, 2019
@bunnymatic bunnymatic force-pushed the chores/remove-bootstrap branch 2 times, most recently from 2744d11 to 2c0924f Compare December 11, 2019 19:48
@bunnymatic bunnymatic changed the title DRAFT : Remove bootstrap Remove bootstrap Dec 11, 2019
@bunnymatic
Copy link
Contributor Author

bunnymatic commented Dec 11, 2019

In preparation for the question "What do we do for back-office/quick-start pages if we have no real css framework/story?", I have the following.

As mentioned above, one major drawback of adding Bootstrap at this stage is that it includes an opinionated reset/normalize. If development is done with this in place, and later Bootstrap is removed, extra work will be required to replace/re-engineer that reset.

Additionally I think that CSS frameworks to solve this back-office or quick-start problem (like any of the following: Ant Design, MaterialUI, Bootstrap, Foundation, Spectre, PureCSS) are relatively easy to add after the fact.

As an organization, we used to leverage Bootstrap far more than I have seen over the past few years. More recently I've seen migration to frameworks that are more in tune with Angular or React projects which require a different kind of inclusion (often it's more like a component library that provides CSS with some kind of CSS in JS solution).

Given that move, I believe that leaving Bootstrap out of this template and letting the team choose their framework of choice based on the projects design constraints is better.

@eskfung
Copy link
Contributor

eskfung commented Dec 12, 2019

I love the thrust of this PR. Would you want to highlight the aforementioned CSS frameworks in our Readme to give more guidance for the follow-up steps?

Copy link
Contributor

@mattbrictson mattbrictson left a comment

Choose a reason for hiding this comment

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

This is looking good!

I chatted with the Front-End Practice Group and the consensus seems to be that while we don't want to endorse Bootstrap as our default out-of-the-box experience, there still may be occasions where a team will want to opt into Bootstrap.

As @eskfung mentioned, I think it would be helpful to point to how to add Bootstrap for those that still want it. Probably 90% of C5 projects won't opt into Bootstrap, but for those that do, some pointers would be helpful.


header.header
.container
a.header__home href="/" App Prototype

main.container role="main"
- flash.each do |name, msg|
.alert.alert-dismissable class=alert_class(name) role="alert"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we drop .alert-dismissable and the alert_class helper? It doesn't seem like there are any style definitions for those classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure. i debated about removing this entirely, but the notion of having a place to put an alert seemed like a nice thing. so ... i'll dump the extra classes.

Comment on lines +1 to +6
$white: #ffffff;
$gray-40: #404040;
$gray-99: #999999;
$gray-cc: #cccccc;
$gray-fa: #fafafa;
$black: #000000;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍


header.header
.container
a.header__home href="/" App Prototype
Copy link
Contributor

Choose a reason for hiding this comment

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

.header__home doesn't seem to be defined. Do we need it here?

@bunnymatic
Copy link
Contributor Author

@eskfung @mattbrictson I agree about trying to answer the "what if i want bootstrap?"
The reason I haven't yet added it to the readme was that I wondered about how many of those cases do we want to support?

What if I want PureCSS/Foundation/MaterialUI/SuperCoolNewCSSThing?

I'm fine with adding notes just for Bootstrap since it'll be pretty easy (cuz I just ripped it out) and because it's the thing we're removing here. Do you think that's enough? Or is there any reason to add a couple answers to "What if i want ?" that should also be added here.

@mattbrictson
Copy link
Contributor

I'm fine with adding notes just for Bootstrap since it'll be pretty easy (cuz I just ripped it out) and because it's the thing we're removing here. Do you think that's enough?

I personally think that is enough. I think documenting the other frameworks is a bit of scope creep for this PR.

@bunnymatic
Copy link
Contributor Author

bunnymatic commented Dec 17, 2019

We (@mattbrictson @eskfung and @bunnymatic) discussed where to put the notes about "what if i want bootstrap?" and agreed that we will add a "next steps" section in the readme on the carbonfive/raygun repo. There we can suggest ways for folks to get bootstrap easily into the project after it's been zapped by raygun.

Additionally, we'll add notes to that effect in the print_next_steps_carbon_five method also in that repo.

@bunnymatic bunnymatic force-pushed the chores/remove-bootstrap branch 2 times, most recently from fb4657d to 05bca46 Compare December 17, 2019 01:24
bunnymatic pushed a commit to carbonfive/raygun that referenced this pull request Dec 17, 2019
Problem
-------

A new version of carbonfive/raygun-rails template will likely not
include Bootstrap by default. carbonfive/raygun-rails#285

Solution
--------

Add notes about how to bring Bootstrap into the project quickly so
for those folks who like having a CSS framework in place, they can
get started quickly.

Changes
-------

* Update the README with `Next Steps` section
* Add `Add Bootstrap` section to next steps with instructions
* update the `print_next_steps` message to point to these instructions

This PR
bunnymatic pushed a commit to carbonfive/raygun that referenced this pull request Dec 17, 2019
Problem
-------

A new version of carbonfive/raygun-rails template will likely not
include Bootstrap by default. carbonfive/raygun-rails#285

Solution
--------

Add notes about how to bring Bootstrap into the project quickly so
for those folks who like having a CSS framework in place, they can
get started quickly.

Changes
-------

* Update the README with `Next Steps` section
* Add `Add Bootstrap` section to next steps with instructions
* update the `print_next_steps` message to point to these instructions
@bunnymatic
Copy link
Contributor Author

This will get merged in concert with carbonfive/raygun#141

Copy link
Contributor

@mattbrictson mattbrictson left a comment

Choose a reason for hiding this comment

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

I'd say this is ready to merge alongside carbonfive/raygun#141! 🚢

bunnymatic pushed a commit to carbonfive/raygun that referenced this pull request Jan 2, 2020
Problem
-------

A new version of carbonfive/raygun-rails template will likely not
include Bootstrap by default. carbonfive/raygun-rails#285

Solution
--------

Add notes about how to bring Bootstrap into the project quickly so
for those folks who like having a CSS framework in place, they can
get started quickly.

Changes
-------

* Update the README with `Next Steps` section
* Add `Add Bootstrap` section to next steps with instructions
* update the `print_next_steps` message to point to these instructions
Jon Rogers added 4 commits January 2, 2020 12:01
Problem
-------

Bootstrap, though it can help to get things rolling, it is not ideal in
the modern day where we often move rapidly to a Javascript
library/framework that has CSS-in-JS or some other scheme to manage
styling.

Including bootstrap includes (implicitly) it's own reset CSS that is
fine at first.  But if you later decide to remove Bootstrap, you may
find that your components no longer look right because you've lost that
implicit reset.

Solution
--------

Remove bootstrap

Changes
-------

* Remove bootstrap from package.json
* update the stylesheets we have to be a bit simpler but still rendering
a header and footer
@bunnymatic bunnymatic merged commit f1365b4 into master Jan 2, 2020
@bunnymatic bunnymatic deleted the chores/remove-bootstrap branch January 2, 2020 20:05
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

Successfully merging this pull request may close these issues.

3 participants