-
Notifications
You must be signed in to change notification settings - Fork 106
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/Ui view handle by Vue.js #678
Conversation
Add Vue js to Atk with two components.
… into feature/vue-js-refactoring
Codecov Report
@@ Coverage Diff @@
## develop #678 +/- ##
=============================================
+ Coverage 72.69% 72.94% +0.25%
- Complexity 1870 1924 +54
=============================================
Files 102 105 +3
Lines 4421 4558 +137
=============================================
+ Hits 3214 3325 +111
- Misses 1207 1233 +26
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## develop #678 +/- ##
============================================
- Coverage 72.69% 43.8% -28.9%
- Complexity 1870 1911 +41
============================================
Files 102 105 +3
Lines 4421 4639 +218
============================================
- Hits 3214 2032 -1182
- Misses 1207 2607 +1400
Continue to review full report at Codecov.
|
props: { | ||
url: String, | ||
initialValue: String, | ||
id: Number, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think ID should be a inside URL as a sticky get. The reasons are:
- there may not be ID, we might want to edit something that has no ID, like "status" of a currently logged-in user.
- ID is not something that component can change as it wishes.
- If we introduce a special way to protect "get" arguments by signing them with limited-time token, this may not apply on "ID" and would require a separate protection.
- similarly - form does not have "ID" as a separate get argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On other hand - perhaps we can pass OTHER arguments, for example if i have multiple inline fields on a page and i want them all to communicate with the same callback JS. Or ability to reference value of other field when submitting.
Perhaps the reason for ID field is so that we can use this component inside table, which would kinda justify the "ID" field. I'd prefer to have either "args" or nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Id value was not mandatory and thought it could be good to have. I removed it.
@@ -1,4 +1,4 @@ | |||
import atkPlugin from 'plugins/atkPlugin'; | |||
import atkPlugin from './atk.plugin'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why atk is not is plugins ? I'm not saying to move it, just curious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure what you mean here. atk.plugin.js is the base class for all other plugins. ex: class ajaxec extends atkPlugin...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gotcha!
|
||
let atkComponents = { | ||
'atk-inline-edit' : atkInlineEdit, | ||
'atk-item-search' : itemSearch, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this have to be populated for all new components? does not seem very universal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vue instance need to have it's component register when you created it. This is one way of doing it and it is ok for component that atk managed. For external component, there would be another way for registering them like the createVue method in Vue service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at some point if we have a vue create guide, should mention that then ;)
} | ||
|
||
/** | ||
* Created a Vue component and add it to the vues array. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
description is unclear. Does that mean that other than those existing 2 components others would need to invoke this method to register themselves? is the name "createAtkVue" miss-leading? maybe not, just raising a question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Component defined externally would have to use this method to create the Vue instance.
* @param event | ||
* @param data | ||
*/ | ||
emitEvent(event, data = {}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unclear what this does and how to use it. No doc or example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would allow for component define in seperate Vue instance to be able to talk to each other.
For example, I use this is saasty for updating the add-ons category when using the item-search component of the add-on market page. The button emit an event that tell the item-search component the category needed to include in it's query.
Been said, the mechanism is in place but I do not have proper use case yet to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but i mean - this need documentation.
* | ||
* @return \atk4\ui\jsToast | ||
*/ | ||
public function jsError($message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is that consistent with $form->error() ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update to use field validation in model.
src/Component/ItemSearch.php
Outdated
*/ | ||
public function getQuery() | ||
{ | ||
return $_GET['_q'] ? $_GET['_q'] : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if multiple search fields present? can they be nested? wouldn't this conflict?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to accept multiple item search. Also doing via __atk_reload argument.
public function setModelCondition($m) | ||
{ | ||
$q = $this->getQuery(); | ||
if ($q && ($_GET['__atk_reload'] ? $_GET['__atk_reload'] : null) === $this->reload->name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think App should have a method for those magic get arguments. Like here: https://github.com/atk4/ui/blob/develop/src/App.php#L264
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just to make sure we are setting up the model for the proper reload view.
$reloadId = $this->reload; | ||
} | ||
|
||
$this->js(true, (new jsVueService())->createAtkVue('#'.$this->name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If VUE is built-in component of ATK, then this line should be simpler:
$this->vue('atk-item-search', ['reload'=>............... ]);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean adding a vue method inside View class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes.
* Emit an event that other component can listen too. | ||
* Allow Vue instances to talk to each other. | ||
* | ||
* This output js: atk.vueService.emitEvent("eventName", {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no examples given.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No example to show yet. Will come later.
… into feature/vue-js-refactoring
To be documented in #683 |
Updating initial prop variable name so it make more sense. update method and comment accordingly
@romans - just push a quick update. |
Guys, stop mentioning me. Thanks! :) |
Codecov Report
@@ Coverage Diff @@
## develop #678 +/- ##
=============================================
+ Coverage 72.69% 72.94% +0.25%
- Complexity 1870 1924 +54
=============================================
Files 102 105 +3
Lines 4421 4558 +137
=============================================
+ Hits 3214 3325 +111
- Misses 1207 1233 +26
Continue to review full report at Codecov.
|
… into feature/vue-js-refactoring
Implementation of Vue.js with Ui view.
Atkjs File name
Demo in vue-composent.php