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

fix: app config page padding property is properly interpreted as shorthand #2720

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jfmcquade
Copy link
Collaborator

@jfmcquade jfmcquade commented Jan 16, 2025

PR Checklist

  • PR title descriptive (can be used in release notes)

Description

  • Fixes an issue where the app_config property LAYOUT.page_padding would not be correctly interpreted as a shorthand CSS property for setting the padding on multiple sides, e.g. 24px 0 or 24px 32px 10px 24px
  • Updates inline_padding variant of the display group component
    • Previously, the size of the padding was read from the same ionic padding variables that are set from the app config level, meaning that if the page padding was set to 0, the padding applied to the inline_padding variant would also be 0.
    • This variant was originally introduced for convenience when dealing with sticky display groups, which ignore page padding by default (see Kuwait Theme: Add Background Image to Display Group #2589).
    • In theory, this variant shouldn't be necessary as it should accomplish the same thing as a single line of custom styling (namely padding-inline: 24px, for example). However, as well as providing a convenient alternative to adding custom styling, the custom styling approach is currently broken, see [BUG] Padding applied to display group through style_list is applied twice #2709.
    • In resolving [BUG] Padding applied to display group through style_list is applied twice #2709, it seems difficult to avoid unwanted knock-ons, since there may be production deployments which make use of the styling at both levels. We need a way to style padding-free pages immediately, therefore it seems like an acceptable short-term solution to leverage the existing inline_padding variant.
      • An alternative would be to set the custom padding-inline to be half the desired value, knowing that it will be applied twice on the rendered component

Dev notes

Because ionic components use the shadow dom, we must rely on the exposed CSS variables to customise the styling. In the case of padding, the shadow element .inner-scroll gets padding applied through 4 exposed CSS variables: --padding-top, --padding-bottom, --padding-start and --padding-end, see screenshot below. In order to set these variables from our authored page_padding value, which defines all 4 via in shorthand syntax, we must manually extract the different values.

Screenshot 2025-01-16 at 17 47 08

Git Issues

Closes #2710

Screenshots/Videos

feat_app_layout_padding:

Screenshot 2025-01-16 at 19 11 36 Screenshot 2025-01-14 at 15 54 02 Screenshot 2025-01-16 at 19 16 30 Screenshot 2025-01-16 at 19 16 39

Copy link
Member

@chrismclarke chrismclarke left a comment

Choose a reason for hiding this comment

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

Thanks @jfmcquade , really nice utility to have and think it's a lot nicer casting the css-style values to the custom ionic properties.

Appreciate having the tests as well, looks like it covers a sensible number of cases and the outputs are as I would expect.

There could possibly be a case made for extracting these at the compiler level, although given there is nothing in the naming convention to indicate the config value is special in some sort of way, and the calculation is incredibly lightweight also happy to keep as-is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants