e107inc / e107

e107 Bootstrap CMS (Content Management System) v2 with PHP, MySQL, HTML5, jQuery and Twitter Bootstrap. Issue Discussion Room: https://gitter.im/e107inc/e107
https://e107.org
GNU General Public License v3.0
321 stars 213 forks source link

Suggestion - separate core templates #4021

Closed Jimmi08 closed 4 years ago

Jimmi08 commented 4 years ago

Templates now work.

Problem with core templates is that one template file should solve legacy, bootstrap3 and now bootstrap4 templates.
Yes, you can manage this with file and IFs, but it's harder to copy the needed template to theme templates (always removing old stuff).

Bootstrap3 templates should be in subfolder bootstrap3, bootstrap4 templates in subfolder bootstrap4. And anytime when BOOTSTRAP is defined (3 or 4), core templates would return templates from those folders (3 or 4).

Jimmi08 commented 4 years ago

@Moc @CaMer0n Could you get me your opinion on this? Now BOOTSTRAP and THEME_LEGACY are correctly used (if not, it is a bug, but their purpose is clear). All templates can be the same for now (like nothing is changed), but it would be easy to fix them later.
This way there would be fewer theme templates and easier fix (improving) core templates. Thanks

Just to be clear what I am suggesting: inside getTemplate() - if BOOTSTRAP ==3 then look at first into templates/bootstrap3/ folder, then to templates as before. BOOTSTRAP ==4 templates/bootstrap4/ then templates... It could be done without real moving templates... only cleaned templates would be moved.

CaMer0n commented 4 years ago

@Jimmi08 I like the idea. Especially with Bootstrap5 coming up also. It will keep the templates much cleaner.

Moc commented 4 years ago

Maybe this will serve as a foundation for being more flexible with the integrations of frameworks such as bootstrap, allowing also using other frameworks in the future.

Jimmi08 commented 4 years ago

Just release 2.3.0 beta at first...

But it would be useful if there is way how saved legacy core templates this way (I mean table layout, not variables. - something like - THEME_LEGACY and folder legacy). I will rewrite them, don't worry.

@Moc in this case you need something like template subset in theme class. Maybe this is a better solution that BOOTSTRAP constant. So if theme->template_subset is bootstrap3, then it will look in templates/boostrap3/ folder in core and in plugins. So it will have no influence on older themes.

Just thinking about it. I am sure you find way that simplified actual getTempate() code...

CaMer0n commented 4 years ago

Maybe this will serve as a foundation for being more flexible with the integrations of frameworks such as bootstrap, allowing also using other frameworks in the future.

Not so much, because all of the shortcodes, classes and javascripts are hardcoded for bootstrap.

I needed this change to properly fix the user profile navigation, so it has been added for v2.3.0

@Jimmi08 We can talk about moving some of those generic bootstrap4 templates to the core now if you wish.

Jimmi08 commented 4 years ago

I can't find the issue where was discussion about using $ instead JQuery. So just note, Boostrap5 will not support JQuery anymore, so new challenges.

Jimmi08 commented 4 years ago

@CaMer0n Do the Legacy templates have to use variables only or will they work with arrays too?

Jimmi08 commented 4 years ago

https://github.com/e107inc/e107/pull/4161

CaMer0n commented 4 years ago

@Jimmi08 Legacy templates are using variables only. They are literally the "legacy template format" which used $VARIABLES = "WHATEVER";