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

Configure the content-header of template #918

Merged
merged 9 commits into from
Jun 8, 2021

Conversation

warquia
Copy link
Contributor

@warquia warquia commented Jun 5, 2021

| Question | Answer
| ----------------------- | [-----------------------]
| Issue or Enhancement | Enhancement
| License | MIT

What's in this PR?

Allowing you to generate or not a content-header, thus allowing at 1024x768 resolutions gains more screen space,

Checklist

  • I tested these changes.
    Git

Copy link
Collaborator

@resslinger resslinger left a comment

Choose a reason for hiding this comment

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

Please add a fallback, if the config var is not set.

With fallback
Copy link
Contributor Author

@warquia warquia left a comment

Choose a reason for hiding this comment

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

add fallback

@warquia warquia requested a review from resslinger June 5, 2021 21:03
@dfsmania dfsmania self-requested a review June 5, 2021 23:57
@dfsmania
Copy link
Collaborator

dfsmania commented Jun 6, 2021

@warquia what classes did you have added to the new config option you have created to get from picture 1 to picture 2. If the goal is to not shown the content-header when the section is not used, we can do like next:

{{-- Content Header --}}
@hasSection('content-header')
    <div class="content-header">
        <div class="{{ config('adminlte.classes_content_header') ?: $def_container_class }}">
            @yield('content_header')
        </div>
    </div>
@endif

Also, note when you do:

<div class="{{ config('adminlte.classes_content_header_main' , 'content-header') }}">

the default value will only be used when the configuration option do not exists. So, when defining the option to null it will inject a null on the class and may result on undesired behavior. You can read laravel docs for details. For this case, I believe it will be better to use:

<div class="{{ config('adminlte.classes_content_header_main') ?: 'content-header' }}">

So, when config('adminlte.classes_content_header_main') evaluates to a falsy value the 'content-header' class will be used.

Also, next time avoid using the master branch of your fork when contributing. Give a fast read to contribution guidelines.

@warquia
Copy link
Contributor Author

warquia commented Jun 7, 2021

Good morning everyone, really has-section is the best approach, I will correct the pull-request with your analysis.
By the way, I'm new to git so I'm going to send this pull in master yet but next time I'll follow the guidance using a branch

Thank you all

a better approach was taken
A better approach was taken
Copy link
Collaborator

@dfsmania dfsmania left a comment

Choose a reason for hiding this comment

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

Please, do not delete config/adminlte.php file. Just use git checkout config/adminlte.php to restore it to the original one, then commit that one. That way you will rollback the first modifications you have done to that file.

@dfsmania
Copy link
Collaborator

dfsmania commented Jun 7, 2021

@resslinger @warquia This PR is ok for me now...

@resslinger resslinger merged commit a325af0 into jeroennoten:master Jun 8, 2021
This pull request was closed.
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.

3 participants