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

Replace skeleton grid with bootstrap grid #2767

Conversation

jacobherrington
Copy link
Contributor

@jacobherrington jacobherrington commented Jun 8, 2018

Refs #2758

The issue
Solidus frontend currently relies on _skeleton.scss, which is a grid system using fixed widths. It is outdated (last updated in 2011). It might confuse or deter newcomers because it is not mobile friendly and difficult to iterate on.

What this PR changes
We added only the grid system from Bootstrap 4.1.1 (bootstrap-grid.css), which should feel familiar to most newcomers and makes it very easy to drop in the full Bootstrap framework if someone wants to extend the existing frontend. On the other hand, it is almost as easy to extract bootstrap-grid.css if someone wants to use another solution for styling. This makes the frontend easier to iterate on.

Caveats
There are cases in which we could have made slight design changes, but we left those choices out of this PR and may revisit them in the future. Instead, we chose to make as few design changes as possible. The goal of this PR was to move from a fixed-width CSS grid to a flex-based CSS grid, not redesign the look and feel of the frontend.

jacobherrington and others added 2 commits June 7, 2018 11:17
Replace instances of Skeleton CSS with Bootstrap-Grid CSS in frontend views

Make minor CSS changes in screen.css.scss to take out margin on product display
@jacobherrington jacobherrington changed the title replace skeleton grid with bootstrap grid Replace skeleton grid with bootstrap grid Jun 8, 2018
@gmacdougall
Copy link
Member

Thanks for your work on this. We've been talking about making major changes to the front end for some time, but looking at this change, I think it can provide a decent quality of development life for people who are using the front end as is right now.

I think the big thing we're concerned about is breaking people's existing customization which they have made using skeleton CSS and markup.

For people just starting their stores now, I do think this is a better options for them.

@jacobherrington
Copy link
Contributor Author

I understand the hesitation @gmacdougall, personally I just feel that the presence (and implied reliance) on skeleton does a lot more harm than good.

Given the current state of Solidus frontend perhaps this PR should go unmerged, but I feel that there is a need to revamp the frontend in a more future-proof way in the near future.

Thanks for the feedback!

@jacobeubanks
Copy link

Thanks for your response!

I agree with @gmacdougall and @jacobherrington. Though I think this is an improvement, I'm not sure if this small change is worth breaking frontends.

@tvdeyen
Copy link
Member

tvdeyen commented Jun 30, 2018

We should be able to keep the skeleton grid until the next version of Solidus and remove it later. Stores using it (very unlikely they actually do) have the chance to migrate to a grid system of their liking (even keep using skeleton by copy it into their app).

We should even be able to print out a deprecation warning with Sass if people are still using our skeleton file.

@ericsaupe
Copy link
Contributor

I like the idea of deprecating it with an eye to a future release. Solidus 3.0 would have the new styles for example since a full point release usually means things are going to break. Out of the box Solidus should look good and look good on mobile.

@jacobherrington
Copy link
Contributor Author

Going to close this until I can revisit removing Skeleton in a better way.

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.

None yet

5 participants