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 "vertical layout" template #3

Merged
merged 41 commits into from
May 17, 2022

Conversation

cavasinf
Copy link
Collaborator

@cavasinf cavasinf commented Dec 3, 2021

Description

Add the "vertical" Tabler layout to the bundle.

Tabler demo link : https://preview.tabler.io/layout-vertical.html

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I updated the documentation (see here)
  • I agree that this code will be published under the MIT license

@cavasinf
Copy link
Collaborator Author

cavasinf commented Dec 3, 2021

As I can see, <head>, .navbar-nav and .page-wrapper are the same between horizontal layout and vertical layout.

If you think this template is correct,
I can't try to make a _base-layout template instead of duplicates parts in common in vertical and horizontal layout.

@kevinpapst
Copy link
Owner

Can you share a screenshot - what is a vertical layout?

@cavasinf
Copy link
Collaborator Author

cavasinf commented Dec 3, 2021

@kevinpapst Demo links from tabler.io and screnshot from your bundle.

Vertical menu

Vertical menu with navbar

Here's a responsive menu:
image

@kevinpapst
Copy link
Owner

Aha, understand. Yeah, let's try to make it one file. Otherwise changes need to be carried over, what will be forgotten at some point...

@cavasinf
Copy link
Collaborator Author

cavasinf commented Dec 3, 2021

There is a transparent theme too: https://preview.tabler.io/layout-vertical-transparent.html

But there's no variable in tabler.yaml to configure that mode (true/false).
I don't want to break everything right now OR make it complicated.

@cavasinf
Copy link
Collaborator Author

cavasinf commented Dec 3, 2021

Aha, understand. Yeah, let's try to make it one file. Otherwise changes need to be carried over, what will be forgotten at some point...

OK, I'll make it into one file.
I'll commit in the same branch, so same merge request

@cavasinf
Copy link
Collaborator Author

cavasinf commented Dec 3, 2021

  • tabler.yaml Added new required [enum] option layout
    • horizontal (default)
    • vertical
  • Pooling "horizontal" and "vertical" layout into layout.html.twig
  • Didn't touch at RuntimeExtensionTest.php

@cavasinf
Copy link
Collaborator Author

cavasinf commented Dec 3, 2021

❔ Questions ❔

  1. Layouts are into templates/includes/layout/, did you prefer in another folder/location?

@kevinpapst
Copy link
Owner

I don't think we will end up with dozens of includes, so templates/includes/layout_horizontal.html.twig would be enough for me.

@kevinpapst
Copy link
Owner

kevinpapst commented Dec 3, 2021

General feedback: I know the wording is based on Tabler demo, but I'd prefer horizontal vs vertical "navigation" and not "layout". As that kind of interferes with boxed and fluid layout.

cavasinf and others added 3 commits December 3, 2021 12:34
Co-authored-by: Kevin Papst <kevinpapst@users.noreply.github.com>
Co-authored-by: Kevin Papst <kevinpapst@users.noreply.github.com>
templates/layout.html.twig Outdated Show resolved Hide resolved
@cavasinf cavasinf added Feature Feature requested Status: Needs Review Not under investigation labels Dec 10, 2021
@cavasinf
Copy link
Collaborator Author

cavasinf commented Dec 10, 2021

❗ Note/Warning ❗
Do not add xxx_before or xxx_after twig blocks to header , nav-bar, aside, ...

The CSS from Tabler.io uses tilde ~ to define styles.
See: https://github.com/tabler/tabler/blob/02c45fc09d020dd424f526384163ab01ddb58b8f/dist/css/tabler.css#L8009

When adding HTML elements, we can brake those selectors.

@kevinpapst
Copy link
Owner

I would say: that is up to the developer to configure properly: so README is enough.

Can you please create a branch in the demo application so I can test this ... I failed miserably last time I tried 😞

@cavasinf
Copy link
Collaborator Author

cavasinf commented Mar 8, 2022

Demo application doesn't works on my side (Docker environment).
And I'm not the administrator of my PC (Remote desktop), can't install symfony CLI, composer, PHP binaries, ...

I've no privilege to open a discussion in that repo, do you want me to open issue about it ?

@kevinpapst
Copy link
Owner

Wow, sounds as if your company makes the developer life real fun ^^

Jokes aide, I think it is best to keep the demo app in sync with PRs here, so we

  • have a living documentation and
  • can test the changes directly in an environment that has no influences from our apps

I am not a docker user, so I cannot provide a docker file, but I assume it shouldn't be to complicated

And I activated discussions for the demo repo.

@kevinpapst
Copy link
Owner

kevinpapst commented Mar 8, 2022

Could you please summarize BC breaks / necessary changes in the initial post?
I just added your branch to the demo app and some parts of the navbar disappeared (haven't checked further yet).

EDIT: the blocks in {% embed '@Tabler/embeds/navbar.html.twig' %}{% endembed %} cannot be overwritten anymore.

@cavasinf
Copy link
Collaborator Author

cavasinf commented Mar 9, 2022

EDIT: the blocks in {% embed '@Tabler/embeds/navbar.html.twig' %}{% endembed %} cannot be overwritten anymore.

Ah.. yes ...
Same problem as here : #3 (comment)

Do we remove the include file and duplicate the code to allows override ?

@kevinpapst
Copy link
Owner

Yeah, if there is no other way we have to duplicate it.

@tacman
Copy link
Contributor

tacman commented Mar 26, 2022

My branch of the demo uses calvin's vertical layout:

https://github.com/tacman/TablerBundle-Demo/tree/tac

What's missing/broken that prevents this from being merged? And is this the right place for the discussion?

@cavasinf
Copy link
Collaborator Author

@tacman Only override of blocks in '@Tabler/embeds/navbar.html.twig' is missing.

I need to try this PR in the Tabler demo.
From memories, I don't think there's any BC breaks or other missing functionalities.

@tacman
Copy link
Contributor

tacman commented Mar 26, 2022 via email

@cavasinf
Copy link
Collaborator Author

@tacman

I should fork cavisin's fork, make changes, and submit them there?

Depend of what changes you want to make, tell me.

The next question is how it should be implemented, a "layout"
key in the config, with the ability to override it

We have already discuss that functionality with kevinpapst.
There's a big problem with twig, twig doesn't allow block override in embedded from extend.
See research here : #3 (comment)

ATE, your views should extend from the provided layouts

{% extends '@Tabler/layout-horizontal.html.twig' %}

OR

{% extends '@Tabler/layout-vertical.html.twig' %}

See : https://github.com/kevinpapst/TablerBundle/blob/efaf141278845dabceba6b37bde9567785be9292/docs/layout.md


Just push what is missing in that PR,
I've tested in the TablerBundle-Demo and everythings looks fine.

Waiting on @kevinpapst to approve.

Florian added 2 commits March 26, 2022 16:09
# Conflicts:
#	templates/layout.html.twig
@cavasinf
Copy link
Collaborator Author

Hey @kevinpapst, long time not see.

I'm starting a new Client project with TablerBundle, and I want that vertical layout in it.
What is missing/not working to merge it ?

@kevinpapst
Copy link
Owner

@cavasinf I do not know, sorry 😄 too long ago and too many other projects in between, since my last review.
I want to have it as well in my project and will look into it this week.

@cavasinf
Copy link
Collaborator Author

Yeah it would be nice, because I still have my personal GitHub link in the composer.json project.
Production environment for my client will come soon, and I'm starting two other projects.

Since I'm not alone to work on it, personal token will not be the solution any longer (for devs or servers)

Friendly reminder, last time I've tested in the Demo project, everything looks clean:
kevinpapst/TablerBundle-Demo#8

kevinpapst
kevinpapst previously approved these changes May 17, 2022
Copy link
Owner

@kevinpapst kevinpapst left a comment

Choose a reason for hiding this comment

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

Added some minor changes:

  • remove global variable layout_type
  • don't show badge on first level in vertical menu

Awesome stuff, thanks @cavasinf 👍

I will release once I could test it in my app. Demo has no issues anymore!

@kevinpapst kevinpapst removed the Status: Needs Review Not under investigation label May 17, 2022
@kevinpapst kevinpapst merged commit 2584dc1 into kevinpapst:main May 17, 2022
@kevinpapst
Copy link
Owner

Wohoo 🎉 🚀

@cavasinf cavasinf deleted the layout-vertical branch May 17, 2022 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Feature requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow dev to choose navbar color light/dark (class)
3 participants