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 Bartik with Responsive Bartik #72

Closed
alexweber opened this issue Sep 14, 2013 · 10 comments
Closed

Replace Bartik with Responsive Bartik #72

alexweber opened this issue Sep 14, 2013 · 10 comments

Comments

@alexweber
Copy link

Since we're stuck with it for now, it might as well be responsive, right?

PR removes the original bartik theme and removes the "responsive_" prefix from responive_bartik: backdrop/backdrop#56

Or do we want to keep the name as "responsive_bartik"?

Or do we not want this at all?

@quicksketch
Copy link
Member

Thanks @alexweber! This looks pretty good. Feedback:

  • Remove LICENSE.txt. We already state that all code is GPL in the root readme.
  • The name is still "Responsive Bartik" in the .info file. Let's keep it just "Bartik".
  • A few other suggestions inline in the patch.

Where did this theme come from? Is it pulled from a D7 repository or somewhere from D8? We have to be careful that wherever it came from, it's probably not fully API-compatible with what we have in Backdrop right now (for example the change from $language->language to $language->langcode looks suspicious to me.)

@quicksketch
Copy link
Member

And we need to get tests passing again after this change.

If you haven't already, I would suggest enabling Travis CI on your own branch at http://travis-ci.org. That will allow you to see if the tests are passing in your own repository on every commit, rather than needing to submit a pull request to see the result.

@alexweber
Copy link
Author

@quicksketch thanks man, will do! the code used is from the latest stable release of the contrib backport: https://drupal.org/node/2018161 (not sure about that weird language one though)

@alexweber
Copy link
Author

Feedback addressed, commits appended to original PR, waiting for Travis to do it's thing

@carwin
Copy link
Member

carwin commented Nov 14, 2013

Since the Travis build failed two months ago this was a little behind.

I updated @alexweber's pull request a little bit to catch things up to the current 1.x branch and fixed some merge conflicts here: backdrop/backdrop#96

I also squashed his commits to make the git log look a little cleaner.

@carwin
Copy link
Member

carwin commented Nov 14, 2013

So even after cleaning up all the merge conflicts and such, the build is still failing miserably. It may be that we just need to start from scratch on this issue if we even want to.

The original source for this theme is http://drupal.org/project/responsive_bartik which is still in beta2. If Bartik is serving our current purposes I don't know that we should spend the time on this. We may be better off just rolling our own theme anyway.

It would be nice to have a responsive default theme.

@scotself
Copy link

scotself commented Mar 5, 2014

I started out adding a new issue but seems this is entirely related, so I'm tying in to this one instead.

I don't know about you guys, but one of the first things I ALWAYS do on Drupal sites is install Admin Menu (or Admin Menu Toolbar style), disable overlay and standard toolbar, shortcuts, the dashboard...I think that's it. Anyway, I'd primarily like to advocate for a nicer primary menu (check out WP's new base theme if you want an excellent example) that is mobile-first. With D8 coming mobile-first out of the box, it will be important to get a nice default theme that's responsive for backdrop too. I'd be happy to work on this if we can get some psds to work from and such and just roll our own theme or continue working off responsive_bartik. Whatever everyone wants to pursue...

@quicksketch
Copy link
Member

We don't yet have an issue for replacing/updating the admin toolbar. I think that would be an excellent place for you to jump into development if you're interested @scotself. We've already removed all the other modules you mentioned (Overlay, Shortcuts, and Dashboard).

Regarding this issue (making Bartik responsive), we're taking a bit of a more comprehensive approach to responsive layouts in the Layout issue (#86). If we succeed there, layouts will be provided by modules rather than themes, effectively meaning shipping responsive layouts would make all themes responsive (though CSS for sizing images and the content itself would still be needed). The layout issue is a bit of a read, but for the time being I think this issue may be postponed.

@quicksketch
Copy link
Member

We don't yet have an issue for replacing/updating the admin toolbar.

I made a new issue for this with an outline of functionality at #189

@quicksketch
Copy link
Member

With Layouts landed in #86 all themes, including Bartik and Stark, are now responsive in their layout. We don't require porting an entire theme at this point, but checking that Bartik is responsive in ways necessary to function well (e.g. putting max-width on images) would be a good improvement. Let's close this issue as the basic concept here of porting the D7/D8 theme is unnecessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants