Skip to content

allow custom breakpoints #44

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

Merged
merged 9 commits into from
Aug 28, 2020

Conversation

plandolt
Copy link
Contributor

@plandolt plandolt commented May 31, 2020

I use sass bootstrap and define custom breakpoints. This PR will allow to overwrite the ?$GLOBALS['BOOTSTRAP_GRID']['breakpoints'] variable to add/change the breakpoints.

  • The dynamic number of grid sizes should be applied in an dca / event listener
  • The configuration has to be be part of the contao_bootstrap config.
  • Then entering the theme context, the theme setting should be used to override the default configuration
  • The lowest size, which does not use the prefix needs to be customizable as well
  • Please check the coding styles
  • Fix fetching grid sized in DCA

Needs contao-bootstrap/core#31

@dmolineus dmolineus assigned dmolineus and plandolt and unassigned dmolineus Jun 2, 2020
@dmolineus dmolineus added this to the 2.3.0 milestone Jun 2, 2020
@dmolineus
Copy link
Member

I like the feature. To follow the concepts of the contao-bootstrap extension I'd like to ask you for adjusting the implementation:

The idea of contao-bootrap v2 is that you can customize settings on theme level. Instead using a global setting, it should be customizable in the theme settings (right now you can customize the number of grid columns already)

Acceptance criterias:

  1. The dynamic number of grid sizes should be applied in an dca / event listener
  2. The configuration has to be be part of the contao_bootstrap config. Default values would be:
    grid:
      sizes:
        - xs
        - sm
        - md
        - lg
        - xl
      default_size: xs
  3. Then entering the theme context, the theme setting should be used to override the default configuration See
    public function buildThemeConfig(BuildContextConfig $command): void
    {
    $context = $command->getContext();
    if (!$context instanceof ThemeContext) {
    return;
    }
    $theme = ThemeModel::findByPk($context->getThemeId());
    if ($theme && $theme->bs_grid_columns) {
    $config = $command->getConfig()->merge(
    [
    'grid' => [
    'columns' => (int) $theme->bs_grid_columns
    ]
    ]
    );
    $command->setConfig($config);
    }
    }
  4. The lowest size, which does not use the prefix needs to be customizable as well (called it default_size in the example above)
  5. Please check the coding styles

@plandolt
Copy link
Contributor Author

PR is now ready

@plandolt
Copy link
Contributor Author

@dmolineus Kannst du den PR bei Gelegenheit prüfen?

@dmolineus
Copy link
Member

@scuben Ich habe den PR noch auf meiner Todo-Liste, kann aktuell aber noch nicht abschätzen, wann ich dazukomme.

 - Drop dynamic code from dca config file
 - Ensure that all available fields are created
 - Automatically create the database fields
 - Avoid error for non existing themes
Replace whole configuration instead of merge with a partial replace
 - Ensure that valid field names are inserted
 - Drop empty and duplicates
 - Use first column if no default grid is selected
@dmolineus
Copy link
Member

@scuben Danke für den PR und sorry für die Lange Pause. Ich habe mir die Implementierung angeschaut und noch einige Sachen behoben, die potentiell Probleme bereitet hätten:

  • Type Error wenn keine Themes existieren
  • DB Felder mussten manuell angelegt werden
  • Invalide Feldnamen waren möglich

Darüber hinaus habe ich die Implementierung so geändert, dass der PR contao-bootstrap/core#31 nicht notwendig ist. Der Ansatz des PRs im Core gefällt mir nicht, da das Verhalten potentiell auf Subebene des Keys unterschiedlich sein kann. Daher erzeuge ich nun ein neues Konfigurationsobjekt.

Kannst du bitte nochmal checken, ob alles für dich passt, dann release ich es zeitnah.

Copy link
Member

@dmolineus dmolineus left a comment

Choose a reason for hiding this comment

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

Die deutsche Übersetzung wirf auf Transifex gepflegt, wird daher beim nächsten Update verlorgen gehen. Ich lasse es dennoch mal vorab drin.

@dmolineus dmolineus merged commit ef36978 into contao-bootstrap:master Aug 28, 2020
@dmolineus
Copy link
Member

Thanks @scuben

@plandolt plandolt deleted the dynamic-breakpoints branch April 1, 2022 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants