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

CMS Theme import and management #211

Merged
merged 89 commits into from
Dec 19, 2019
Merged

Conversation

boehsermoe
Copy link
Member

@boehsermoe boehsermoe commented Jul 26, 2019

@boehsermoe boehsermoe changed the title Display active theme in debug toolbar theme Jul 28, 2019
@boehsermoe boehsermoe changed the title theme CMS Theme import and management Jul 28, 2019
@nadar
Copy link
Member

nadar commented Jul 29, 2019

So this will require version 3.0 release

@nadar nadar added this to the 3.0 milestone Jul 29, 2019
@nadar nadar self-requested a review July 29, 2019 07:38
@boehsermoe boehsermoe requested a review from nadar December 9, 2019 09:00
Copy link
Member

@nadar nadar left a comment

Choose a reason for hiding this comment

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

I would say we are almost there.

It would be nice to have an option to render the toolbar file, then we can at least ensure there are no syntax errors. Would be great! Thanks.

Thanks for the work, looking forward to merge.

src/admin/importers/ThemeImporter.php Show resolved Hide resolved
src/models/Theme.php Outdated Show resolved Hide resolved
src/models/Theme.php Show resolved Hide resolved
src/models/Theme.php Show resolved Hide resolved
@boehsermoe
Copy link
Member Author

It would be nice to have an option to render the toolbar file, then we can at least ensure there are no syntax errors. Would be great! Thanks.

What do you mean with this? Where could syntax errors occur?

@nadar
Copy link
Member

nadar commented Dec 10, 2019

in the view file, so it thought it would be nice to just render the view file in the controller. (cause there are changes to the toolbar).

$ctrl = new DefaultController();
$this->assertNotNUll($ctrl->actionIndex());

something like this.

@boehsermoe
Copy link
Member Author

@nadar I made a simple test for the toolbar rendering

@nadar
Copy link
Member

nadar commented Dec 17, 2019

Im terms of coverage, this looks good! Today i will make a review regarding phpdoc and coding standards, everything else looks good for me now 👍

Copy link
Member

@nadar nadar left a comment

Choose a reason for hiding this comment

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

please fix those phpdocs, afterwards we can merge. nice!

Maybe we should also adjust the guide and provide a screenshot about the new theme manager? (but of course in antoher PR in luyadev/luya)

https://luya.io/guide/app-themes

src/frontend/Module.php Show resolved Hide resolved
src/models/Theme.php Outdated Show resolved Hide resolved
@codeclimate
Copy link

codeclimate bot commented Dec 18, 2019

Code Climate has analyzed commit 973ff13 and detected 85 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 20
Duplication 65

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

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

View more on Code Climate.

@nadar nadar merged commit 07a9773 into luyadev:master Dec 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants