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.

Sign up for GitHub

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

Jump to bottom

D8CORE-7654: Fixed the title to left on a full width page. No change to a layout none page. #373

Open
wants to merge 7 commits into
base: 9.x
Choose a base branch
from
Open

D8CORE-7654: Fixed the title to left on a full width page. No change to a layout none page. #373

wants to merge 7 commits into from

Conversation

jenbreese
Copy link
Contributor

@jenbreese jenbreese commented Feb 13, 2025 •
edited
Loading

READY FOR REVIEW

Summary

  • On a full width page, the page title would move around a bit when changing browser size.

Review By (Date)

  • Next release

Criticality

  • Normal

Urgency

  • Normal

Review Tasks

Setup tasks and/or behavior to test

  1. Check out this branch
  2. Rebuild Cache and import config drush cr ; drush ci
  3. Navigate to a full width page
  4. Change the size of the browser and verify that the title remains left aligned with the content below
  5. Verify no changes to the layout none page

Site Configuration Sync

  • Is there a config:export in this PR that changes the config sync directory?

Front End Validation

Backend / Functional Validation

Code

  • Are the naming conventions following our standards?
  • Does the code have sufficient inline comments?
  • Is there anything in this code that would be hidden or hard to discover through the UI?
  • Are there any code smells?
  • Are tests provided? eg (unit, behat, or codeception)

Code security

General

  • Is there anything included in this PR that is not related to the problem it is trying to solve?
  • Is the approach to the problem appropriate?

Affected Projects or Products

  • Does this PR impact any particular projects, products, or modules?

Associated Issues and/or People

- D8CORE-7654

Resources

@jenbreese jenbreese requested a review from pookmish February 13, 2025 23:18
pookmish
pookmish requested changes Feb 28, 2025
View reviewed changes
modules/stanford_profile_styles/lib/scss/component/node/stanford_page.layout.full-width.scss Outdated
}

.main-region {
&.flex-10-of-12 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This defeats the purpose of 10 of 12 columns. The better solution is to update the full width layout itself. We might not be able to achieve the same solution you have here, but changing it to a "centered" container with default width would match other pages.

See what you can do on /admin/structure/layouts page.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Thank you for the tip. I have it working and undid the change above too. I'll update the Prs.

@jenbreese jenbreese mentioned this pull request Mar 4, 2025
Closed
14 tasks
@github-actions github-actions bot added the patch label Mar 17, 2025
@jenbreese
Copy link
Contributor Author

@pookmish I updated the PR as suggested. Let me know what you think.

pookmish
pookmish reviewed Mar 21, 2025
View reviewed changes
modules/jumpstart_ui/templates/layouts/one-column.html.twig Outdated
@@ -20,7 +20,6 @@
{% if settings.columns|render is not empty and settings.columns != "default" %}
{% if region_attributes.main is iterable %}
{% set main_attributes = main_attributes.removeClass('flex-12-of-12').addClass(settings.columns) %}
{% set attributes = attributes.addClass('centered-content') %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because with it is there, the h1 moves back and forth as the browser width changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then the h1 is pushed against the left side of the screen, making it worse than before.
I also feel like this could have a more negative impact than we are aware. This template is used in many places outside the basic page. I would want a full 100% review of every place and every scenario that this touches. It's too risky IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct. I answered too quickly earlier. I'll work on it on Monday. I feel awful this afternoon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed it. Will you take a look on Monday?

@jenbreese jenbreese mentioned this pull request Mar 23, 2025
Merged
14 tasks
@jenbreese
Copy link
Contributor Author

@pookmish I took all the changes out of this PR so I can just close it, right?

@pookmish
Copy link
Contributor

@jenbreese yes. you can close the PR and delete the branch if you feel appropriate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pookmish pookmish pookmish requested changes

Assignees

@pookmish pookmish

Labels
patch size/xs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jenbreese @pookmish