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

Draft yiisoft/view assets, and view no merge. #50

Merged
merged 32 commits into from
Aug 28, 2019
Merged

Draft yiisoft/view assets, and view no merge. #50

merged 32 commits into from
Aug 28, 2019

Conversation

terabytesoftw
Copy link
Member

Q A
Is bugfix? ✔️
New feature?
Breaks BC?
Tests pass? ✔️
Fixed issues #49

@terabytesoftw
Copy link
Member Author

terabytesoftw commented Aug 23, 2019

[x] AssetBundleTest. Done.
[-] AssetConverterTest. In process.
[x] ThemeTest. Fixed.
[x] ViewTest. Fixed.
[x] WebViewTest. Fixed.
[-] Widgets . In process: Are we going to keep the widgets configuration type array ?

If you can fix the travis configuration.

Thks,

@samdark
Copy link
Member

samdark commented Aug 23, 2019

In process: Are we going to keep the widgets configuration type array ?

I was thinking about alternatives but haven't found better ones yet :( If you have any ideas, feel free to post them for discussion.

There is interesting forum discussion about Html helper https://forum.yiiframework.com/t/using-functions-for-html-generation/126814/24 with the idea of having $this-> style of view plugins. Not sure it suits widgets though.

Also, instead of config, config-methods could be used to make configuration in IDEs more handy.

If you can fix the travis configuration.

It is not broken as far as I can see.

@terabytesoftw
Copy link
Member Author

I honestly believe that we should maintain the array configuration in the widgets and static helpers, I think that injecting everything through the container di would be equal to Yii2, in my opinion the views are only the representation of the result, and using static helpers would be fine, for on the other hand we would leave things like Yii2 like helpers and widgets that do not need a great design, I was reviewing the OptionsResolver Component of Symfony, it could be a good alternative to better control the matrix configurations.

And with respect to travis it was if the configuration of .travis.yml was already standardized.

composer.json Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
composer.json Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
phpunit.xml.dist Outdated Show resolved Hide resolved
composer.json Show resolved Hide resolved
src/View/View.php Show resolved Hide resolved
@terabytesoftw
Copy link
Member Author

terabytesoftw commented Aug 24, 2019

We should create a repository of exceptions, we need to have more specific exceptions, those of php are very generic.

What is the replacement for:

Yii::debug(), Yii::error()

I can update the configuration of .travis.yml to that of Yiisoft/Template

Please Add repo scrutinizer, coverage tests AssetBundle (100%), AssetConverter (94%),

Thks,

@samdark
Copy link
Member

samdark commented Aug 24, 2019

We should create a repository of exceptions, we need to have more specific exceptions, those of php are very generic.

Create ones you need right in this repository.

What is the replacement for:
Yii::debug(), Yii::error()

  1. Require "psr/log": "^1.1", in composer.json.
  2. Type-hint against Psr\Log\LoggerInterface $logger.
  3. $this->logger->error(), $this->logger->debug()

I can update the configuration of .travis.yml to that of Yiisoft/Template

Please do.

@samdark
Copy link
Member

samdark commented Aug 24, 2019

Please Add repo scrutinizer, coverage tests AssetBundle (100%), AssetConverter (94%),

Done.

@terabytesoftw
Copy link
Member Author

terabytesoftw commented Aug 26, 2019

@samdark What do you think about injecting the aliases and assetconverter into the bundle __construct() to use it in the same bundle.

Example:

<?php
/**
 * @link http://www.yiiframework.com/
 * @copyright Copyright (c) 2008 Yii Software LLC
 * @license http://www.yiiframework.com/license/
 */

namespace Yiisoft\Yii\Bootstrap4;

use Yiisoft\Aliases\Aliases;
use Yiisoft\Asset\AssetBundle;
use Yiisoft\Asset\AssetConverter;

/**
 * Asset bundle for the Twitter bootstrap css files.
 *
 * @author Qiang Xue <[email protected]>
 */
class BootstrapAsset extends AssetBundle
{
    public $sourcePath = '@npm/bootstrap/scss';

    public $css = [
        'bootstrap.css',
    ];

    public $publishOptions = [
        'only' => [
            'bootstrap.css',
            'bootstrap.css.map',
        ],
    ];

    public function __construct(Aliases $aliases, AssetConverter $assetConverter)
    {
        $assetConverter->commands['scss'] = [
            'css', 'sass {from} {to}'
        ];
        $assetConverter->convert('bootstrap.scss', $aliases->get('@npm/bootstrap/scss'));
    }
}

Review the documentation please.

@samdark
Copy link
Member

samdark commented Aug 26, 2019

I was always looking at bundles as configuration containers, not as anything that actively does things.

@samdark samdark added the status:code review The pull request needs review. label Aug 26, 2019
@terabytesoftw
Copy link
Member Author

It is correct but to access the converter you will have to call it from the controller, and do the conversions, I think it is a good idea in the bundle, since it is converted to publish, on the other hand I would also agree if that task is left that the developer do it on your own using sass, gulp, and any tool for it.

@samdark
Copy link
Member

samdark commented Aug 26, 2019

Conversion may happen in many ways. Yii 2 has two out of the box ways for it:

  1. By asset manager in the web app.
  2. By console command that was combining/compressing assets.

Additionally community developed tools that generated configs for grunt/gulp and other native JavaScript builders from bundles.

I think it would be a bad idea to force specific way of conversion in the bundle itself because of that.

.travis.yml Outdated Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
src/Asset/AssetManager.php Outdated Show resolved Hide resolved
src/Asset/AssetManager.php Outdated Show resolved Hide resolved
src/View/View.php Outdated Show resolved Hide resolved
src/View/WebView.php Outdated Show resolved Hide resolved
src/View/WebView.php Outdated Show resolved Hide resolved
@terabytesoftw
Copy link
Member Author

terabytesoftw commented Aug 27, 2019

For widgets we must continue with the configuration of arrays since we must allow them to be easy to configure and can be changed dynamically.

Example:

/**
  * @var array $widgetOptions
  */
private $widgetOptions = [
    'setId()',
    'setItems()',
 ];

/**
 * setConfig widget options.
 *
 * @param array $config
 *
 * @throws \InvalidConfigException
 *
 * @return void
 */
public function setConfig(array $config): void
{
    foreach ($config as $key => $item) {
        if (in_array($key, $this->widgetOptions)) {
            $this->$key($config['$key']);
        } else {
            throw new \InvalidConfigException("Widget options: ($key) no exist.");
        }
    }
}

protected function setId(string $value): void
{
    $this->id = $value;
}

And we change the init() method to beforeRun()

docs/guide/README.md Outdated Show resolved Hide resolved
docs/guide/assetbundles.md Outdated Show resolved Hide resolved
docs/guide/config-composer-json.md Outdated Show resolved Hide resolved
docs/guide/config-composer-json.md Outdated Show resolved Hide resolved
docs/guide/config-composer-json.md Outdated Show resolved Hide resolved
@samdark
Copy link
Member

samdark commented Aug 27, 2019

@terabytesoftw is it overall ready for thorough review? Any things that should be done before that?

@terabytesoftw
Copy link
Member Author

AssetBundle, AssetManager and AssetConverter work well I have tried them I am happy with that, now I would also like to fix the widgets and leave the complete operating package, of course you also have to fix i18n to establish everything well.

@samdark
Copy link
Member

samdark commented Aug 27, 2019

How about doing that in subsequent pull request? This one's diff is already close to impossible to review (but I can handle that).

@terabytesoftw terabytesoftw marked this pull request as ready for review August 27, 2019 14:34
@terabytesoftw
Copy link
Member Author

How about doing that in subsequent pull request? This one's diff is already close to impossible to review (but I can handle that).

Done.

src/View/Theme.php Outdated Show resolved Hide resolved
src/Asset/AssetManager.php Show resolved Hide resolved
src/View/View.php Show resolved Hide resolved
src/View/View.php Show resolved Hide resolved
src/View/View.php Show resolved Hide resolved
src/View/WebView.php Outdated Show resolved Hide resolved
src/Asset/AssetBundle.php Show resolved Hide resolved
*/
public function init(): void
public function publish(AssetManager $am): void
Copy link
Member

Choose a reason for hiding this comment

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

It is still valid AssetBundle publishes itself with the help of asset manager. Maybe asset manager should do it completely?

@samdark samdark added status:under development Someone is working on a pull request. and removed status:code review The pull request needs review. labels Aug 28, 2019
@samdark samdark removed the status:under development Someone is working on a pull request. label Aug 28, 2019
@samdark samdark merged commit e039ceb into yiisoft:master Aug 28, 2019
@terabytesoftw terabytesoftw deleted the TeraAsset branch August 28, 2019 14:48
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.

2 participants