Make it easier to add full width backgrounds #4582
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This way we can set `$text` to `$black`, as we intended to.
javierm
force-pushed
the
full_width
branch
2 times, most recently
from
25e4e5e
to
8430ba3
Compare
javierm
changed the title
javierm
changed the title
javierm
force-pushed
the
full_width
branch
8 times, most recently
from
66623ed
to
2b76bfc
Compare
Instead of defining them for all headers and then overwriting them, we can add it just to the body of the public layout.
javierm
force-pushed
the
full_width
branch
4 times, most recently
from
230de4e
to
030440e
Compare
We weren't using a global maximum width for the <body> element because
we wanted the background of some elements to cover the whole screen. If
the body didn't cover the whole screen, then we would have to find a way
to extend the background beyond the limits of the body.
Elements can take the whole screen width using a width of 100 viewport
width (vw) units, which weren't as widely supported when CONSUL
development started as they are today.
However, there's a gotcha will vw units; they don't take into account
the vertical scrollbars browsers add when scroll is needed. That means
that an element with a width of 100vw would cause a *horizontal*
scrollbar when the vertical scrollbar appears on the screen. So
approaches like this one wouldn't work:
```
body {
margin-left: auto;
margin-right: auto;
max-width: $global-width;
}
@mixin full-background-width {
&::before {
margin-left: calc(50% - 50vw);
margin-right: calc(50% - 50vw);
}
}
```
We could add `overflow-x: hidden` to the body to avoid the horizontal
scrollbar. However, on certain screens sizes that could cause some
content to disappear if there isn't enough horizontal space for all the
elements.
If we tried some other solution based on using `max-width` with `margin:
auto` on the <body> element, it would result in a body having a fixed
width and a variable margin (depending on whether there's a scrollbar).
So it wouldn't be possible to set a negative margin on child elements
based on the margin of the body, because that margin would be different
depending on the existence of a scrollbar.
So, instead, we're adding a fixed margin to the body, which depends on
the viewport width and the font size of the <html> element. With this
approach, when a vertical scrollbar appears, the margin of the <body> is
still the same; what changes is its width. That means we can set a
negative margin on child elements based on the margin of the <body>. No
horizontal scrollbar will appear.
Note we're slightly duplicating the code by using two variables
(`$body-margin` and `$full-width-margin`) to do the same thing. We could
simply use `$body-margin` and then use `calc(-1 * #{$body-margin})` in
our `full-width-background` mixin. We aren't doing so because some old
versions of the Android browser and Internet Explorer can't handle this
operation. Since our whole layout is based on these properties, in this
case supporting old browsers is quite important.
For similar reasons we're using a breakpoint instead of using the
`max()` function like: `Max(0px, calc(50vw - #{$global-width / 2}))`. At
the time of writing, `max()` is only supported in about 91% of the
browsers.
With this change, we no longer need to add `row` elements to make sure
we don't exceed the maximum width; the <body> element takes care of
that.
Also note banners sometimes have a full background and sometimes they
don't, depending on which page they appear. We're adding specific rules
for them.
Finally, the code for full width borders is a bit brittle; sometimes we
want the border to cover an element, and sometimes we don't. For
example, we had to slightly change the way the border of the "tabs" in
legislation processes is rendered. Without these changes, the borders
wouldn't overlap as we intended. We also had to add a `z-index` to
navigation links so their bottom outline is visible when they're
focused. The recommendations have a border with the same color as the
background so it's painted on top of the border of the `help-header`
section.
We were using these rules in order to set the maximum width of an element to `$global-width`. However, since we now do so in the <body> element, there's no need to apply these rules to "rows". Note we're adding `overflow: hidden` to the budget subheader. That's because it only contains `float` element inside, and we're now missing the `.row::before` and `.row::after` rules which make sure float elements are rendered properly.
We couldn't do it before because we didn't have a way to add full width backgrounds to elements which were inside a ".row"-like element.
These element had no columns inside and the row classes had only been added to give them a maximum width. That's no longer necessary since now the body has that maximum width.
In the case of the public layout, the row element was originally there so the content of the top links had a maximum width. Since now the body has that maximum width, we no longer need the row element. In the other layouts I guess the row elements were added because there were float elements inside them. We can use a flexbox layout instead and these elements are no longer necessary. This also makes the layout more robust when there isn't enough space on one line for both the language selector and the external links. Note we're using `flex-grow: 1` to make one element appear on the left of the screen and the other one on the right. It would be easier to use `justify-content: space-between`. However, there's a bug in Internet Explorer and old versions of Firefox; they include the absolutely-positioned `::before` element we use to set the full width background when calculating where to position the elements. The bug was fixed in Firefox 52 (released in 2017).
javierm
force-pushed
the
full_width
branch
2 times, most recently
from
8136a3d
to
741ab86
Compare
javierm
force-pushed
the
full_width
branch
3 times, most recently
from
a1d44db
to
7024bee
Compare
taitus
approved these changes
Jul 8, 2021
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -43,7 +43,7 @@ def check_image | |||
|
|
|||
| dimensions = Paperclip::Geometry.from_file(image.queued_for_write[:original].path) | |||
|
|
|||
| errors.add(:image, :image_width, required_width: required_width) unless dimensions.width == required_width | |||
| errors.add(:image, :image_width, required_width: required_width) unless dimensions.width <= required_width | |||
There was a problem hiding this comment.
Layout/LineLength: Line is too long. [112/110] (https://rubystyle.guide#max-line-length)
javierm
force-pushed
the
full_width
branch
3 times, most recently
from
934a58d
to
2054a38
Compare
Since the top-bar already includes a layout positioning the elements, these classes are redundant and actually confusing, since the element floating to the right was on the left. This solves a problem where the outline wasn't correctly displayed when focusing on the logo using the keyboard. Firefox was displaying two vertical lines together above the logo, while recent Chrome versions displayed the outline to the right of the logo.
This way it will be announced as a landmark to screen readers.
Using `flex` instead of a fixed width for the navigation, the elements take all the available space when the search form isn't present. That wasn't the case before and produced a strange effect on medium-sized screens. This way we also align the search to the right.
javierm
force-pushed
the
full_width
branch
2 times, most recently
from
47e7695
to
fef38bb
Compare
Instead of adding the padding to each individual element inside the container, why not adding padding to the container itself? The answer is "because we want the background of the children elements to take the width of the whole screen". But this generates either HTML cluttered with elements to add padding or repetitive padding definitions in the CSS. So now we only define the padding once, and when an element requires a full width background or border, we use the `full-width-background` mixin. In this case the code is a bit more complex because the header is also used in the dashboard and admin layouts: * In the public layout, the body has a margin, so we include the mixin to take margin into account * In the dashboard layout, the header itself has a margin, so we include the same mixin * In the admin layout, the headet doesn't have a margin but gets the whole width, so in this case we include the mixin which dosen't take the margin into account In the future, the idea is to apply this principle to the <body> element and remove the `@include grid-column-gutter` in the CSS as well as the `small-12 column` classes in the HTML. Note we use the `calc()` function inside the mixin instead of using it in the `$full-width-margin` variable. That way we avoid nested `calc()` operations, which don't work in Internet Explorer. Also note we're using `flex-grow: 1` to make one element appear on the left of the screen and the other one on the right. It would be easier to use `justify-content: space-between` (which is actually the default for the top-bar element). However, there's a bug in Internet Explorer and old versions of Firefox; they include the absolutely-positioned `::before` element we use to set the full width background when calculating where to position the elements. The bug was fixed in Firefox 52 (released in 2017). Finally, we're removing the padding from our logo. In order to allow logos like the new one and at the same time provide backwards compatibility to logos in existing CONSUL installations, we're relaxing the validation rule for the logo width.
javierm
force-pushed
the
full_width
branch
2 times, most recently
from
aa3888b
to
b8ee325
Compare
We can give the padding to the whole page instead of giving it to individual elements. On the minus side, now padding in the SDG pages is not the same as the padding in the homepage, so we need to add an extra padding to the participation feeds in only one of these cases.
taitus
approved these changes
Jul 9, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objectives
Visual Changes
Before these changes
After these changes
Notes
These changes, alongside using flexbox instead of floats for layout, make all
class="row"elements in the public layout deprecated. We shouldn't add any new elements with that class.