openaustralia / morph

Take the hassle out of web scraping
https://morph.io
GNU Affero General Public License v3.0
461 stars 74 forks source link

Size of header search made consistent for medium width screens #1141

Closed Anishka0107 closed 6 years ago

Anishka0107 commented 7 years ago

This pull request aims to fix the issue #585 The width of the header search becomes small at the wider end of the screen, i.e., for widths between 768px to 1200px. For small (<768) and large (>1200) width screens, the search bar size was already consistent. Now, search bar width has been made consistent for medium width screens as well.

For medium width screen : searchbarfixed

Anishka0107 commented 7 years ago

@henare Added the new commit, but using nav-search-form as ID name.

equivalentideas commented 7 years ago

Hey @Anishka0107 , I've actually got a bit more feedback on this 😜

Of the three css rules added, only one of them is actually creating the intended change:

@media (min-width: $screen-sm-min) {
  #nav-search-form .input-group .input-group-addon, #nav-search-form .input-group .input-group-btn, #nav-search-form .input-group .form-control {
    width:0;
  }
}

The other too rules, setting the width of the .form-control and container .input-group to 100% don't actually make a difference, so we should remove those. We want to avoid adding any redundant styles because they make future changes much harder.

I worked that out by removing the rules and testing if it made a difference.

The next thing, is we're targeting three elements here with the selectors #nav-search-form .input-group .input-group-addon, #nav-search-form .input-group .input-group-btn, #nav-search-form .input-group .form-control—but it's actually only one of them that needs to be targeted to have the desired effect #nav-search-form .input-group .input-group-btn. We can remove the other two selectors from this statment.

Next is the style rule that's being applied.

As your change shows, the reason the form changes width is that the .input-group-btn goes from having basically no width, to being width auto. This is all coming from bootstrap the framework we used here.

From Bootstrap, the .input-group-btn has a width of 1%. You can see that using the web inspector and targeting that element. And then there's another rule in there to make sure the forms in the navbar shrink when the browser window gets to a certain width, so it fits over on the right-hand side of the wide screen navbar.

.navbar-form .input-group .input-group-btn {
    width: auto;
}

It's got other selectors in there, but this is the one that makes a difference.

In morph we've made the breakpoint when the menu goes from small screen style to wide screen style wider than in default bootstrap, and that's where this bug comes from. The form gets made small too early.

So, all we need to do here, is make sure that style doesn't get applied to screens that are smaller than our big menu breakpoint—for our navbar search form. Because we're overriding the framework here, we want to do it in the most focused, lightweight way possible. Otherwise future changes can get really tricky (Right To Know is kinda in that state 😞 ). Because all our custom styles get compiled after the bootstrap framework styles, we don't need an id to overpower their css selector, we can just depend on the cascade and use the same selector as them .navbar-form .input-group .input-group-btn. That means you can remove the id from the form :)

The other thing is that the original style is width: 1% rather than width: 0;. I'm not sure why that is, but we rely on the framework to work out all the clever hacks for us, so we should use their solution.

So we end up with the style here being.

.navbar-form .input-group .input-group-btn {
    width: 1%;
}

We also need to apply the media query to get this to apply where we want. You've used @media (min-width: $screen-sm-min) to apply the style to everything below a specific width. I think though that because we want to make sure the form stays wide until it gets to that main mobile breakpoint, I think it would be better to a max-width and that breakpoint specifically @media (max-width: $grid-float-breakpoint).

So now we've got the only change as:

.navbar-form .input-group .input-group-btn {
  @media (max-width: $grid-float-breakpoint) {
    width: 1%;
  }
}

Because this style is an override, in other words it builds on the framework, we should note that in a comment. So if we upgrade the bootstrap or something and this style makes a bug, we can understand why we added it (and if we can safely remove it):

// Override bootstrap navbar form width so it
// stays full width until the main menu breakpoint
.navbar-form .input-group .input-group-btn {
  @media (max-width: $grid-float-breakpoint) {
    width: 1%;
  }
}

Because this change is about the layout of the header, I think it's best placed in app/assets/stylesheets/global/_site-header.scss rather than app/assets/stylesheets/search.scss. The other styles it could interact with would be around the header so that feels safer to me.

As @henare says, I'm a pedant for style 😜 . Note that I've nested the media query under the selector, rather than the other way round. That's the style used in this app :)

Sorry for such a rant! I'm just a bit out of the css game and wanted to walk through it for my own sanity. Also, contributing css to an app is hard, and modifying a framework is hard :) Does this ramble make sense? 😄

mlandauer commented 6 years ago

Closing this (without merging) as there hasn't been any activity on this PR in a while