Olivero: Center align content (instead of left align)

This is per design. Please see the designs listed on the project page.

I agree with @ambuj_gupta, the left-centered design seems odd ... I can't remember seeing anything but centered lay-outs for quite a number of years now. It seems like the norm. I am adding a patch as an example. Perhaps the background-drops can be faded out?

Centered example, without background-drops

Updating the title for clarity.

Challenge: List a few popular web sites with left aligned content.

Hi,

I am creating a patch for this issue.

Please review the patch.

Let me know for any suggestions.

Thanks.

Thanks! The patch applies, but since I don't know how to build a complete Olivero theme, I had to manually add the change to layout.css. I am not sure if I am a fan of the white stripes, which the padding: 0px 20px 0px 20px; adds, especially on the left side ...

Closing this as "works as designed", but moving into the core issue queue. If more people are requesting this, please comment. It is possible to create a setting.

I have created a Merge Request which centers the lay-out.

Having the issue closed might prevent some users from finding it, since the default Status filter is "Open issues" ... So in order to have a free and open debate I would recommend opening it.

I am not sure if I should change Status to trigger a run of the Merge Request? I'll try :)

I now see a mention of tugboat above ("View live preview (suspended) via Tugboat"), I am not sure if it was already there?

Anyway, here is a live preview of the Olivero theme enabled, with the patch applied, pretty cool feature! https://mr42-jkq7tkmwsb3wv3w187sddm26fdwfgg5o.tugboat.qa

EDIT: The tugboat instance seems to reset automatically after a while, so you'll have to enable it yourself.

I have added new patch for 9.2.x.
PLease check the screenshots
Firefox:

Chrome:

Supporting browser:
https://caniuse.com/?search=margin-left%3A%20auto

Thank you all for taking the time to create and submit patches for this issue! Unfortunately, the layout of the Olivero page is designed to be aligned on the left-hand side and there's no intention of adding equal padding from the left and right of the container.

Marking this issue as "Closed (works as designed)" for now.

Unfortunately, the layout of the Olivero page is designed to be aligned on the left-hand side and there's no intention of adding equal padding from the left and right of the container.

I understand. At some point, a decision was made to left align the layout, but surely that decision can be reverted? I again would like to pose this simple challenge:

List a few popular web sites with left aligned content.

If you can't do that, doesn't that signify that the left-aligned design doesn't follow the general, best practice in web design? Making it possible to create a setting as @mherchel mentions would be acceptable as a work-around.

@ressa Thanks for the response! I definitely see your point and I understand that this feature might be something that we need to expose in the Olivero theme settings to give users the ability to change the layout of the page container per install. However, the caveat here is that we'll need to make sure the center alignment layout will not introduce any regressions/issues to the theme.

With that being said, I'm going to re-open this issue and leave it to the community to decide. If you feel strongly about exposing an option for the center alignment layout within the Olivero theme settings, please add a +1 to this issue thread. Thanks!

I think a lot of people's first reaction will be to look for the button to center the layout (well, I did at least, the big empty space on the right of the theme really draws attention to it on an average sized monitor).
If this is a policy thing it would be a bit of a missed opportunity (I think) as the theme looks really nice! :).

+1 for centered lay-out from me as well (no surprise!). And thanks @proeung for being open to let the community weigh in on this issue. I agree with @herrzhull, the design looks really nice, and would be a huge improvement in Drupal.

With that being said, I'm going to re-open this issue and leave it to the community to decide.

Moving to "needs more info" to wait if people comment on this.

Another +1 for centrally aligned canvas. That being said its a simple code override on a per project basis where needed.

New patch attached adds a theme setting to allow center-aligned layout.

Patch applies to the 9.3.x branch, so no interdiff.

Makes sense to have this configurable and a good idea imo, thanks for looking at this.

Some style tweaks to resolve;

themes/olivero/css/components/site-header.pcss.css
 39:5  ✖  Expected rule to come before at-rule with a block   order/order

themes/olivero/css/layout/layout.pcss.css
 24:5  ✖  Expected "margin-right" to come before "margin-left"   order/properties-order

Thanks! @andy-blum for the patch and Thanks @aaron.ferris for the review.

Providing updated patch by resolving linting errors. Kindly review.

credit: @andy-blum

Thanks!

Code wise it looks OK to me.

Patch #29, gives a checkbox in the theme settings to center the layout and center the alignment.

but header opening and closing are showing odd behavior. Please check the video. When the header collapse, the origin is on left most side of the page and it is looking odd.

When opening the sticky header the overflow:hidden comes off before the header slides in. To prevent this behavior we could add a transitionend even to remove the overflow:hidden

A quick one, should we also be updating olivero.schema.yml given there is a new configurable option being added?

I did a re-roll of the patch in the MR, please revise.

Thanks @marcusvsouza, perhaps you could include an interdiff? It makes it easier for everyone to quickly spot the differences:

Using interdiffs is a best practice that saves time and reduces tedium for reviewers by allowing them to focus on just the changes introduced in patch iterations.

https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa...

Here's the re-roll diff, sorry for not include in the past comment!

Verified and tested patches #34 and #38.
The patch does not apply successfully.

Please refer attached screenshots.
Moving to Needs work.

Verified patch #34 it's applied successfully.

Working as expected. It providing an utility to make layout center navigate to /admin/appearance/settings/olivero and check the checkbox in front of Center align site layout from Olivero Utilities.

Before patch SS:

After patch SS:

Can be move to RTBC.

Thanks!

Great! So now we know it works, and there is a work-around.

Fundamentally, the current left-aligned design by default is not in line with the rest of the internet. Perhaps we could consider whether left-aligned or centered design should be the default, according to what the users prefer, and check if a majority prefer one or the other?

So far it seems like @ambuj_gupta, @ressa, @herrzhull and @aaron.ferris prefer a centered design. Feel free to share your preferred alignment :-)

The fly-out menu isn't properly contained when it re-enters the page (see attached). Moving this back to needs work.

I have tried to solve the issue pointed in #45.

Forgot to update status.

Thanks, but the latest patch also seems to remove the setting ...

Re-rolled patch #34, The fly-out menu is fixed now, when it re-enters the page. Attached interdiff for same. Please review.

Setting the site header to `overflow:hidden` prevents using the dropdown menus.

And also partial support of [position: sticky], which should be removed/reworked/returned back to position: fixed; i believe

Remove the overflow and changed it to an animation.
Regarding the position sticky, the line above is position fixed (as a fallback), so I don't see een problem in that.

I applied the patch and tested the changes in the comment #52
The menu it's working properly.

This is the best we've gotten so far but I think a better approach might be to listen for transitionstart and transitionend events to fire and to add an inline style of overflow-X: hidden. Something like this:

slidingHeader.addEventListener('transitionstart', (e) => {
    if (e.target === slidingHeader) {
        headerWrapper.style.overflowX = "hidden";
    }
});

slidingHeader.addEventListener('transitionend', (e) => {
    if (e.target === slidingHeader) {
        headerWrapper.style.overflowX = "";
    }
});

With that we'll prevent both the lateral translation from showing outside the page wrapper as well as preventing the distortion that's coming from the animated scale that's coming in #52.

The only real side effect of the above code will be a brief scrollbar, which we should be able to find a way to hide on all browsers.

@andy-blum: Isn't this out of scope, since the animation is the same as the animation of the menu in the offset page? With the patch of #52, I don't see the headers showing outside the page wrapper when translating.

Is there any reason why patch #54 can't work in Drupal 9.3.x? I'm trying it out and although it applies correctly and the new option appears, the layout doesn't seem to care about that option being ticked

Attached is a modified re-roll of #52 that applies to 9.4.x.

To answer @al0a in #57, my one concern with using scaleX() instead of translateX() for the site header is that it warps the text during the open/closing animation. See the below version with the animation slowed down.

I agree with @andy-blum. I think we should preserve the sliding motion of the top menu without adding distortion (since that's the current behavior of the theme).
I didn't manage to record a video or make a gif, but here are some screenshots during the transaction for comparison.

Current behavior (without patch):

Changed behavior (patch with scale(x)):

If I apply the patch, revert the changes on site-header.css, and check to centralize the page I get the menu to slide without distortion but that brings back the problem from #45. If we could somehow preserve the current slide, but avoid the "overflow" on the left would be ideal.

I brought this issue to the #3271646: Drupal Usability Meeting 2022-04-01 and we got to a consensus that changing the behavior of the to menu from sliding to shrinking would brake consistency. From a UX point of view we should try keep consistency on the menu behavior for a better experience so this patch should only provide a new alignment option but not change the menu behavior.

People who voted for preserving the slide effect: @bejifisher @ckrina @Antoniya @ kimberlly_amaral @tmaiochi (sorry if I forgot someone).

Maybe we could give a try to #56 suggestion.

+ 1 for a centered layout

+ 1 for a centered layout

If the slide effect is blocking progress here, perhaps it could be removed for centered lay-out? I am generally not a fan of animation, so for me it wouldn't be a loss.

re-rolling the patch #60 for 9.5.x

Hello everyone, my proposal in the attached patch:

1. To contain the sliding of the header I used clip-path instead of overflow: hidden or an animation (I used inset() as basic-shape which should be supported by all browsers, except IE, but for Drupal 10 it is not a problem anymore). This way, the drop-down menus of the main nav are not cut off.
2. I also handled the situation where the "Enable mobile menu at all widths" option is set: in this case the off-canvas menu (from the right) stops at the center of the page.
3. Created on 10.0.x, but runs smoothly on 10.1.x too

Please review, thank you.

Thanks for the updated patch @MXT! It applies perfectly against 10.1.x-dev and works well in both Desktop as well as mobile display.

I see a single notice in the Console Output, which could explain why the test doesn't go green here:

Timed out while waiting for element to be present for 5000 milliseconds. - expected "visible" but got: "not found" (5098ms)

https://dispatcher.drupalci.org/job/drupal_patches/154115/console

Also, it would be great if you could include an interdiff file against the patch your changes are made, so other users can follow along with the code changes.

Test fails on Olivero backend settings, so it doesn't seems related to new frontend implementation (or yes?):

Any suggestion?

H,

Tested the latest patch with version 10.1.x but they not working for me .
create a patch Center-align-content-3156754-73.patch, attached the screen shot, after-patch-3156754.png, before-patch-3156754.png

please review the patch.

Sorry @Harish1688, but your patch is too simple. We need to have this as a setting, please see the patch in #70.

Please before posting a patch be sure to read the whole thread and check the work already done by others first, thank you.

@MXT thank your for all your work on this! Looked at core/tests/Drupal/Nightwatch/Tests/Olivero/oliveroColorTest.js though I am not sure why the test fails. As you commented, It might be unrelated to the changes in #70.

Guys, this is getting absurd: I spent more time trying to fix the failing test than implementing the new feature that allows Olivero to be centered.

The beauty is that the test that fails on the backend, has nothing to do with the frontend functionality that works great, as confirmed by others above, so much so that we are using it successfully on an upcoming site (can't share the demo as it is not yet officially public).

The most absurd thing is that the element that the test is looking for exists because moveToElement does not fail, while the waitForElementPresent/Visible in the next line continues to fail. So, first it finds it and then it doesn't find it anymore?

Anyway I don't want to insist on this anymore, first to avoid spamming this thread with various attempts to fix the test, due to the fact that I'm basically working "blind", as the Nightwatch tests don't produce screenshots so that I can visually see where the problem is, and don't even run correctly on DrupalPod (I run them using $ddev nightwatch --tag olivero, but it doesn't work correctly: do you know a better way?). I tried to use DrupalPod because I still couldn't get Nightwatch to run correctly in my usual "docker4drupal" based local environment...

Probably, as often happens, it may be that the failure of the backend test is due to something really trivial, but which I haven't been able to see yet...

At this point I'm asking you all, and in particular Olivero's maintainer, for a suggestion on how best to proceed in this situation.

Thank you.

Thank you for your tireless efforts @MXT. I know the feeling of mysteriously failing tests, one of the reasons I got involved with Lando + Drupal Contributions: To make PHPUnit and Nightwatch testing easier.

With the kind help of @neclimdul, @DuaelFr and @mparker17 Nightwatch in Lando is now working (see Fix PHPUnit invalid cookie domain and failing Nightwatch test #76), so you can run Olivero's Nightwatch tests with these commands:

git clone git@github.com:thinktandem/drupal-contributions.git
cd drupal-contributions
lando rebuild -y
cd web/core
lando nightwatch --tag olivero

I patched and ran the tests, and got feedback in the /web/core/reports/nightwatch folder with images, HTML, JSON files, etc. One error image was a screenshot about SchemaIncompleteException and missing olivero.settings which gave me a clue that missing schema and config might be the problem.

I am attaching the resulting Nightwatch report zipped.

AAAHHH!!! Thank you very much @ressa!!!

So with my:

Probably, as often happens, it may be that the failure of the backend test is due to something really trivial, but which I haven't been able to see yet...

I was absolutely right! ;-D

And yes: this is the demonstration that with the right tools, the goal can be reached sooner and better: I didn't know about this Lando + Drupal Contributions: I absolutely have to find the time to try it.

Thank you for the tip @ressa, much appreciated!

You're welcome @MXT :) And so true about the right tool. Some of the best IT advice I ever got was from Sacha Chua (presentation 2009) to "Sharpen your axe": Find the best tools, and perfect the usage of them.

It's nice with a tangible result of the time me and others have spent trying to get tests working in Drupal Contributions. Since you're familiar with DDEV, you shouldn't find it too hard to get Lando up and running.

Code review completed on gitlab, adding screenshots here

Behavior on scroll

Left aligned:

Center aligned:

Behavior on scroll (slowed down)

Left aligned:

Center alinged:

Theme setting

Dropdown menus

Moving to needs work based on code review suggestions, this is *really* close though!

Updated attributions

For those wishing to test the above implementation, the related patch can be created by clicking on plain diff in the "Issue fork" box at the beginning of this thread.

I reproduce the current link here for convenience:

Plain diff to be applied as a patch

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Since this is adding a new setting it will need a change record + an upgrade path for existing sites. Test should be included.

Also with this new setting it will need test coverage as well.

If anyone wants to take on the upgrade path & tests, you may be interested in following the model set by #3257274: Implement color changing theme settings for Olivero, which also needed those items.

Hello, +1 here also for the centered layout, waiting for the merge.
I have a suggestion, but I am a bit concerned not to postpone the merge, thank you all for the work put into this.
As for the suggestion, In my opinion (I am a graphic designer if that matters :))...

The left margin which is gray with the Rss feed and the above blue rectangle should remain in the left side.
The navigation should be from margin to margin, centered, equally distributed toward the margins of the screen. So the branding part more to the left, the menus more to the right.
I would also renounce that pijama :) looking background and let the background white.
If the background is made white for example, (and for many cases it will be, as the texture can't fit with the feel of certain designs) it is obvious that the gray margin doesn't make any sense to be toward the center, that's why I would let it remain in the left.

So the only part that should be centered is the content, and in the navigation part, the elements (branding and menus) equally distributed toward the margins.

But if this would postpone the merge, it is better to leave it as it is patched right now, because this is a very important feature, that is obviously missing in the Olivero theme, and everything takes such a long time... This is not a question - if there should be or not such a feature. Of course there should be.
PS, Lament :) I love Drupal, but being visually oriented, I waited such a long time for features like layout builder, or good themes (Uikit was one of the best until Drupal 10), we are getting old here... :) many times the tire for some projects was deflated. So it's better that this feature merges as it is patched, but I thought I should add my suggestion. Thank you.

Thanks for sharing @CPxiom, and I very much agree that the sooner this gets committed and released, the better.

You could create follow-up issues for your suggestions about element alignment, as well as optionally disabling the background image?

Page wrapper left align issue is replicated and fixed for all resolutions.

@athyamvidyasagar there is already a working patch (see #87) that is much more advanced and takes into account many other aspects than yours, which is too simple.

Please before posting a patch be sure to read the whole thread and check the work already done by others first, thank you (this is the second time I say this in this thread)

In case you (or someone else) want to contribute to the conclusion of this work, what is missing to do is written in comment #88

Thank you.

Can confirm a patch created from https://git.drupalcode.org/project/drupal/-/merge_requests/3447 works on Drupal 10.2.x
after updating the theme settings.

Any plans to continue work/review on this? Trying the MR on 10.4 it seems to work well. Thanks!