search

luyadev / luya

LUYA is a scalable web framework and content management system with the goal to please developers, clients and users alike.
https://luya.io
MIT License
812 stars 207 forks source link

Luya core theming #1936

Closed boehsermoe closed 5 years ago

boehsermoe commented 5 years ago

this closes #1916 There are still some todo which I will solve it in the next time. But it has a good stability incl. unit tests and should also be backward compatible.

Theme class: extend the yii2 theme component and build the correct $pathMap based of ThemeConfig

ThemeConfig class: Handle the theme.json files

ThemeManager class: Initialize the active theme

This PR required luyadev/luya-composer/pull/12.

nadar commented 5 years ago

First of all: Thanks @boehsermoe for taking care of this.

Let me know as soon as i should make a reivew (next time you could use the "new" draft pull request button), as i can see a lot of todos.

Maybe within this PR you could also add a guide documents this will also help me to understand the mechanism and documentation would have been done along with the new feature.

As far as i can read, this WON'T break the application, so its BC safe?

boehsermoe commented 5 years ago

Thank you , good to know about draft PR. I will remember for next time. And yes I will make a guide for the setup and configuration. As far as I tested it is BC safe, but I will also add a unit test to check this explicit.

boehsermoe commented 5 years ago

@nadar it is done for now. I think most of it is well documented so you can start your review.

There is only one open todo which we should discuss later.

boehsermoe commented 5 years ago

@nadar I have worked through your comments.

  • There is a lot of not covered code with tests, even exceptions should be tested. As the PR is so huge, we need more coverage here.

Many exceptions are not yet covered, but these will come soon.

  • a lot of missing php docs, but this is something we can also adjust at the end before merging. But it would help me a lot to understand the functions. What does the function? Where is it needed?

If you missing somewhere a php docs or if it is so less, just make a comment.

Technically:

  • The concept of APP_THEMES_BLANK is unclear to me. If there is no theme, why we should have this default theme which is pointing to a fix location in the app?

APP_THEMES_BLANK is kicked out.

  • The setup of themes should know whether there are themes to setup or not.

The setup will check now there is a active theme configured.

Thanks for the work! Even i think the yii2 theming mechanism has its problem (widgets? blocks?) i think with some default themes which are looking nice, this could be a very cool feature!

I think so too! Blocks and widget views can also override in a theme. I started here with a luya module included two themes: https://github.com/boehsermoe/luya-themecollection

boehsermoe commented 5 years ago

So i would say if we reach 100% test coverage

@nadar do you think we need also a 100% coverage for core/console/commands/ImportController.php and core/console/commands/ThemeController.php? I think it could be hard to test the cli user inputs like confirm?

boehsermoe commented 5 years ago

@nadar luyadev/luya-testsuite/pull/22 is required for the unit tests

nadar commented 5 years ago

you can run composer update, the testsuite is released

codeclimate[bot] commented 5 years ago

Code Climate has analyzed commit b8a8182f and detected 4 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 4

The test coverage on the diff in this pull request is 100.0% (80% is the threshold).

This pull request will bring the total coverage in the repository to 64.4% (1.8% change).

View more on Code Climate.