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

add set layout option feature #181

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

Conversation

igorveremsky
Copy link

According to official AdminLTE layout documentation:

  • Add feature to set layout option as layout property at AdminLteAsset.php
  • Add feature to load extra assets (js, css, depends) according to specific layout
  • Add feature to get html class by call static method \dmstr\helpers\AdminLteHelper::layoutHtmlClass()
  • Add documentation to README.MD

TODO:

  • feature to set more than one layout options
  • some tests maybe (i`m bad with this)

Related issues:

@schmunk42
Copy link
Member

Looks good, is this BC-compatible by default?

}

$layoutExtraAssetsOptions = self::getLayoutExtraAssetsOptions();
if (array_key_exists($layout, $layoutExtraAssetsOptions)) {
Copy link
Member

Choose a reason for hiding this comment

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

This section could be problematic when bundling assets, since some stuff is loaded dynamically.

Might be an option to register the additional asset files manually via the ExtraAsset... - although this is not a perfect solution.

Copy link
Author

Choose a reason for hiding this comment

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

But this stuff (extra assets) is needed by default for specific layout. Maybe, to do it more flexible we can create AdminLteBowerAsset that adds possibility for developers to connect scripts, styles from bower_components directory at almasaeed2010/adminlte package.

But somehow logic that for specific layout options asset connect extra scripts, styles must be present.

Copy link
Author

Choose a reason for hiding this comment

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

its related to #180

Copy link
Author

@igorveremsky igorveremsky Nov 1, 2018

Choose a reason for hiding this comment

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

I think also solution can be write a notes to docs that some layouts by default adds extra js, css from bower_components

Copy link
Member

Choose a reason for hiding this comment

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

But instead of setting a property, how about simply registering AdminLteFixedLayoutAsset instead of AdminLteAsset?

@igorveremsky
Copy link
Author

@schmunk42 by default layout option equals false so according to this expression this changes is BC compatible. Because, only method run() rewrited, other variables, methods, functions added.

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

Successfully merging this pull request may close these issues.

2 participants