Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kslonka’s picture

valkum’s picture

Status: Needs review » Reviewed & tested by the community

approve.
patch works.

markhalliwell’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/bootstrap_subtheme/less/overrides.less
@@ -305,3 +305,17 @@ div.password-suggestions ul {
+ul.secondary {
+  float: right;
+}
+
+@media (max-width: @screen-sm) {
+  .logo {
+    margin-left: 15px;
+  }
+
+  ul.secondary {
+    float: left;
+  }
+}

Now that Bootstrap is technically mobile first, we should reverse this so it only applies a float on tablet and above.

markhalliwell’s picture

Also, go ahead and take out the logo modifications. We have #2094411: Create new logo which will probably alter this anyway.

valkum’s picture

Should be this way, or am i wrong?

markhalliwell’s picture

Status: Needs work » Needs review
Issue tags: +Needs manual testing

That looks right, yes... but I'd like someone to review this manually.

markhalliwell’s picture

+++ b/bootstrap_subtheme/less/overrides.less
@@ -305,3 +305,11 @@ div.password-suggestions ul {
+@media @tablet, @normal, @wide {

In reality, this should probably just be:

@media @tablet {}

But the variables need to be updated to be a min-width, not set sizes.

Status: Needs review » Needs work

The last submitted patch, bootstrap-fix-navbar-collapsed-2094991-5.patch, failed testing.

valkum’s picture

So the following changes are in this patch.
* fix navbar element floating on mobile displays
* reworked all media queries to be mobile first.

valkum’s picture

Status: Needs work » Needs review
markhalliwell’s picture

Status: Needs review » Needs work

Sorry to do this, but I was in the middle of #2098175: Clean up files and now the structure has completely changed (to be easier for maintaining). Please re-roll your patch against the latest HEAD. Also, be sure to read the documentation in template.php to understand what has changed. Thanks!

valkum’s picture

Thats a lot of new files ^^
But this patch should be fine. Only the menu function moved to theme/menu/...
No big deal.

wundo’s picture

Status: Needs review » Fixed

warning: 1 line adds whitespace errors.

Fixed and Committed!

wundo’s picture

Ohh Valkum, I'm sorry, I forgot to change the commit author.

markhalliwell’s picture

I just fixed a few things, but gave @valkum credit since there was a mishap :)

Committed 29dfb51 to 7.x-3.x.

valkum’s picture

Ohh i missed a MQ. sry. @wundo, no problem.

markhalliwell’s picture

Version: 7.x-3.x-dev » 7.x-3.0-rc1

Automatically closed -- issue fixed for 2 weeks with no activity.

MMTLukas’s picture

Status: Closed (fixed) » Needs review

Status: Needs review » Needs work
markhalliwell’s picture

Issue summary: View changes
Status: Needs work » Closed (won't fix)

Don't retest patches that have already been committed to code (they will always fail).

markhalliwell’s picture

Status: Closed (won't fix) » Closed (fixed)

Oops, wrong status.