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

Feature/js integration #20

Merged
merged 30 commits into from Dec 28, 2016

Conversation

Projects
None yet
2 participants
@romaninsh
Copy link
Member

commented Dec 27, 2016

Following #18 , this is a second step of the integration. Chains now are integrated into the View class and js() and on() methods are implemented.

Test suite has been added and I've also added our first Selenium test!

We're actually running IE7 to test our JavaScript engine.

Travis tests are currently ignored because of http://stackoverflow.com/questions/41337139/allow-phpunit-test-to-access-actual-environment-variable?noredirect=1#comment69877502_41337139

@romaninsh romaninsh added the in review label Dec 27, 2016

@romaninsh romaninsh added this to the 0.7 jQuery Chains milestone Dec 27, 2016

romaninsh added some commits Dec 27, 2016

docs/js.rst Outdated
@@ -30,11 +30,19 @@ with:

.. code-block:: js
<<<<<<< HEAD

This comment has been minimized.

Copy link
@DarkSide666

DarkSide666 Dec 28, 2016

Member

something is wrong here with merging my docs fixes from develop branch. <>develop left here.

* // event of $img.
*
* Produced code: $('#img_id').on('mouseenter', function(ev){ ev.preventDefault();
* $('view1').hide(); });

This comment has been minimized.

Copy link
@DarkSide666

DarkSide666 Dec 28, 2016

Member

proper selector naming $('#view_id').hide()

* @param array|jQuery_Chain|string $code JavaScript chain(s) or code
* @param string $instance Obsolete
*
* @return jQuery_Chain

This comment has been minimized.

Copy link
@DarkSide666

DarkSide666 Dec 28, 2016

Member

should be @return jQuery (not Chain)

* For more information on how this works, see documentation:
*
* @link http://agile-ui.readthedocs.io/en/latest/js.html
*/

This comment has been minimized.

Copy link
@DarkSide666

DarkSide666 Dec 28, 2016

Member

no @param, @return descriptions

/** @var bool add stopPropagation(event) */
public $stopPropagation = false;
/** @var string Array of statements */

This comment has been minimized.

Copy link
@DarkSide666

DarkSide666 Dec 28, 2016

Member

wrong comment

This comment has been minimized.

Copy link
@romaninsh

romaninsh Dec 28, 2016

Author Member

not sure why wrong.

romaninsh added some commits Dec 28, 2016

romaninsh added some commits Dec 28, 2016

all comments addressed

@romaninsh romaninsh merged commit 400c69c into develop Dec 28, 2016

3 of 6 checks passed

codeclimate 26 new issues (5 fixed)
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
codeclimate/coverage 74.04% (+5.5%)
Details
continuous-integration/styleci/pr The StyleCI analysis has passed
Details
continuous-integration/styleci/push The StyleCI analysis has passed
Details

@romaninsh romaninsh deleted the feature/js-integration branch Dec 28, 2016

@DarkSide666
Copy link
Member

left a comment

Need to fix some stuff in this (already merged) commit.

@@ -64,6 +64,8 @@ class View implements jsExpressionable
* Path to template. If you don't specify full path
* by starting with '/' then will be prepended by
* default template path.
*
* @var Template

This comment has been minimized.

Copy link
@DarkSide666

DarkSide666 Jan 2, 2017

Member

It's not Template, it's a string!

This comment has been minimized.

Copy link
@romaninsh

romaninsh Jan 3, 2017

Author Member

it's actually string first but is replaced with a template. In a way, it combines function of 'defaultTemplate'. If I specify 'string|Template' then code hinting is not working. Something need to be renamed here.

This comment has been minimized.

Copy link
@romaninsh

romaninsh Jan 3, 2017

Author Member

fixed in f0ae37d

$this->app->init();
}
/**
* In addition to adding a child object, set up it's template
* and associate it's output with the region in our template.
*
* @param $object View New object to add

This comment has been minimized.

Copy link
@DarkSide666

DarkSide666 Jan 2, 2017

Member

should be @param View $objec tNew object to add

This comment has been minimized.

Copy link
@romaninsh

romaninsh Jan 3, 2017

Author Member

Fixed in 096a361

@@ -294,8 +306,6 @@ public function addClass($class)
* Remove one or several CSS classes from the element.
*
* @param string|array $class CSS class name or array of class names

This comment has been minimized.

Copy link
@DarkSide666

DarkSide666 Jan 2, 2017

Member

only string is treated correctly (see explode). Or it should be like this in code:

if (is_string($remove_class)) {
    $remove_class = explode(' ', $remove_class);
}

This comment has been minimized.

Copy link
@romaninsh

romaninsh Jan 3, 2017

Author Member

Fixed in 096a361 and ccb669b

@@ -440,10 +450,10 @@ public function getHTML()
* @link http://agile-ui.readthedocs.io/en/latest/js.html
*
* @param string|bool|null $when Event when chain will be executed
* @param array|jQuery_Chain|string $code JavaScript chain(s) or code
* @param array|jQuery|string $code JavaScript chain(s) or code

This comment has been minimized.

Copy link
@DarkSide666

DarkSide666 Jan 2, 2017

Member

$when can not be array as far as I see in code. Only string|boolean|jQuery

This comment has been minimized.

Copy link
@romaninsh

romaninsh Jan 3, 2017

Author Member

i was planning to make array possible, but perhaps I shouldn't bother.

This comment has been minimized.

Copy link
@romaninsh

romaninsh Jan 3, 2017

Author Member

Dark, where did i say that "when" could be an array? Give me link an i'll fix it there.

@@ -516,7 +526,7 @@ public function on($event, $selector = null, $action = null)
// if callable $action is passed, then execute ajaxec()
//
throw new Exception('VirtualPage is not yet implemented');
$url = '.virtualpage->getURL..';
/*$url = '.virtualpage->getURL..';

This comment has been minimized.

Copy link
@DarkSide666

DarkSide666 Jan 2, 2017

Member

no respective closing */

This comment has been minimized.

Copy link
@romaninsh

romaninsh Jan 3, 2017

Author Member

yah, it's just blocking unfinished code, so it's intended. That area is not reachable anyway.

romaninsh added a commit that referenced this pull request Jan 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.