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.

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

Support globally setting max simple page content width #14819

Merged
merged 8 commits into from
Nov 29, 2024

Conversation

zepfietje
Copy link
Member

Description

Currently it's not possible to globally set the max width for simple pages like registration and login. This pull request adds support for globally configuring the width just like is already possible for regular pages.

Functional changes

  • Code style has been fixed by running the composer cs command.
  • Changes have been tested to not break existing functionality.
  • Documentation is up-to-date.

@zepfietje zepfietje added the enhancement New feature or request label Nov 16, 2024
@zepfietje zepfietje added this to the v3 milestone Nov 16, 2024
@zepfietje zepfietje changed the title Simple page max width Support globally setting simple page max width Nov 16, 2024
Copy link
Member

@danharrin danharrin left a comment

Choose a reason for hiding this comment

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

Thoughts on simpleMaxContentWidth() instead of simplePageMaxWidth()? Existing setting is maxContentWidth()

@zepfietje
Copy link
Member Author

Hmm, ideally the APIs would be maxPageContentWidth and maxSimplePageContentWidth, don't you think? Just "simple" without "page" might not be clear enough.

Regarding "max width" vs "max content width", I'm aware of that inconsistency but just stuck to the existing naming. Happy to change anything where possible :)

@zepfietje
Copy link
Member Author

@danharrin actually I'm not sure if I like this configuration on the panel provider as global config methods. What do you think about introducing a static property on the page classes, $defaultMaxContentWidth?

@danharrin
Copy link
Member

  1. Static properties can only really be set once and not per-request otherwise it breaks Octane
  2. Static properties can't differentiate widths based on which panel is used

Personally I usually only use static properties for BC or when something can't be a panel configuration option

Why don't you like the panel config method pattern?

@zepfietje
Copy link
Member Author

  1. Didn't know about that, good point.
  2. Static properties would have to be set in bootUsing() of the panel config.

I do like the panel config method pattern in general, but the max page content width APIs feel a bit weird since there's two different types of pages. Maybe it's just a naming issue we can easily resolve though.

@danharrin
Copy link
Member

Can you rename to maxSimplePageContentWidth() so I can merge this please?

@zepfietje zepfietje changed the title Support globally setting simple page max width Support globally setting max simple page content width Nov 25, 2024
@zepfietje
Copy link
Member Author

Done! 😊

@zepfietje zepfietje requested a review from danharrin November 25, 2024 10:22
Copy link
Member

@danharrin danharrin left a comment

Choose a reason for hiding this comment

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

Sorry, could this be simplePageMaxContentWidth and getSimplePageMaxContentWidth instead please, so then when you search maxContentWidth both are returned, if you know what I mean

@zepfietje
Copy link
Member Author

Smart, done that!

@zepfietje
Copy link
Member Author

Could you merge and tag a new release, @danharrin?
This just missed the latest release and there's a couple people waiting on this change :)

@danharrin danharrin merged commit c9e458a into 3.x Nov 29, 2024
20 checks passed
@danharrin danharrin deleted the simple-page-max-width branch November 29, 2024 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants