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/Add Form support for containsMany field using MultiLine #784

Merged
merged 47 commits into from
Sep 19, 2019

Conversation

ibelar
Copy link
Contributor

@ibelar ibelar commented Jul 23, 2019

todo

  • update atk4/data/Reference/ContainsMany with new ui and system = false property
    As of now, field using containsMany need to be explicitly set using system property to false in order to appear in form.
    $this->containsMany('Addresses', [Address::class, 'system' => false]); ,
  • update for use with unloaded model
    This code in Form is causing issue: $model = $f->reference->refModel(); because when trying to create containsMany reference model on a new record, ContainsMany::getDefaultPersistence($model) in atk4/data throws exceptions: 'model should be loaded.'
if ($f->type === 'array') {
            $limit = ($f->reference instanceof ContainsMany) ? 0 : 1;
            $model = $f->reference->refModel();
            $fallback_seed = ['MultiLine', 'model' => $model, 'rowLimit' => $limit, 'caption' => $model->getModelCaption()];
        }
  • decide if we keep __atkml value in containsMany record - __atkml is needed for multiline component in order to manage error properly.
  • decide if we include Semantic-UI vue in atkjs package. As of now, if we want to have Multiline works in CRUD or in a form inside a modal, we have to explicitly load semantic-ui Vue package.
$app->useSuiVue();
$app->add(['CRUD'])->setModel($m);

Update

  • add support for limiting number of record.
    ex: $f->addField('test', ['MultiLine', 'model'=> Model::class, 'rowLimit' => 2])
    Therefore containsOne will automatically use MultiLine using limit of 1 record.

  • add support for Dropdown field in Multiline

Screen Shot 2019-07-24 at 5 39 36 PM

  • add model caption (see image below)

  • add support for Dropdown options via 'ui' => 'multiline' property
    $this->hasOne('country_id', [new Country(), 'ui' => ['multiline' => ['dropdown' => ['search' => true]]]]);

usage:

class Client extends atk4\data\Model
{
    public $table = 'client';
    public $caption = 'Client';

    public function init()
    {
        parent::init();
        $this->addField('name');

        //use system = false until atk4\data is update.
        $this->containsMany('Addresses', [Address::class, 'system' => false]); 
    }
}

class Address extends atk4\data\Model
{
    public $caption = 'Address';

    public function init()
    {
        parent::init();
        $this->addField('street', ['required' => true]);
        $this->addField('city');
        $this->addField('country');
        $this->addField('postal_code');
    }
}

$f = $app->add('Form');
$f->setModel(new Client($db));

Screen Shot 2019-07-25 at 9 31 22 AM

@codecov
Copy link

codecov bot commented Jul 23, 2019

Codecov Report

Merging #784 into develop will increase coverage by 0.01%.
The diff coverage is 5.71%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop     #784      +/-   ##
=============================================
+ Coverage      71.24%   71.26%   +0.01%     
- Complexity      2244     2246       +2     
=============================================
  Files            119      119              
  Lines           5325     5325              
=============================================
+ Hits            3794     3795       +1     
+ Misses          1531     1530       -1
Impacted Files Coverage Δ Complexity Δ
src/FormField/MultiLine.php 0% <0%> (ø) 105 <0> (ø) ⬇️
src/Form.php 75.16% <50%> (-0.86%) 70 <0> (+2)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc25357...7560c09. Read the comment docs.

@codecov
Copy link

codecov bot commented Jul 23, 2019

Codecov Report

Merging #784 into develop will decrease coverage by 0.34%.
The diff coverage is 59.82%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop     #784      +/-   ##
=============================================
- Coverage      76.54%   76.19%   -0.35%     
- Complexity      2251     2287      +36     
=============================================
  Files            119      119              
  Lines           5337     5398      +61     
=============================================
+ Hits            4085     4113      +28     
- Misses          1252     1285      +33
Impacted Files Coverage Δ Complexity Δ
src/App.php 82.51% <ø> (-0.31%) 129 <0> (-2)
src/Form.php 73.33% <40%> (-1.33%) 68 <0> (+2)
src/FormField/MultiLine.php 36.92% <60.71%> (+4.67%) 140 <39> (+35) ⬆️
src/jsVueService.php 80% <0%> (-20%) 5% <0%> (ø)
src/TableColumn/FilterModel/TypeString.php 36% <0%> (-2.1%) 8% <0%> (+1%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b5b7ce...1e38da6. Read the comment docs.

ibelar and others added 9 commits July 23, 2019 15:12
use containsOne record with limit of one record.
- set new version number
- fix error when a new field in model containsMany/containsOne was added.
- update release note and version number.
@romaninsh
Copy link
Member

What's the status of this?

@romaninsh romaninsh removed the hangout agenda 🔈 Will discuss on next hangout label Aug 29, 2019
@ibelar
Copy link
Contributor Author

ibelar commented Aug 29, 2019

What's the status of this?

@romaninsh - All is ready on UI side. Waiting for atk4/data to be changed before we can merged.

@acicovic
Copy link
Collaborator

acicovic commented Sep 4, 2019

So what must be done next? Is this tracked somewhere in an issue or PR in the data repository?

@DarkSide666
Copy link
Member

DarkSide666 commented Sep 5, 2019

Please review/merge atk4/data#459 and then data issue should be fixed.
No more need to set system property or work around loading model first.

change function name.
Now include in Bundle
@PhilippGrashoff
Copy link
Collaborator

As data changes are merged, we could merge this one?

Copy link
Collaborator

@PhilippGrashoff PhilippGrashoff left a comment

Choose a reason for hiding this comment

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

LGTM

*
* @returns {boolean}
*/
isLimitReach() {
Copy link
Member

Choose a reason for hiding this comment

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

isLimitReached - better language

@romaninsh romaninsh merged commit f2172ed into develop Sep 19, 2019
@romaninsh romaninsh deleted the feature/multine-containsmany branch September 19, 2019 18:13
@acicovic acicovic mentioned this pull request Oct 5, 2019
6 tasks
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.

5 participants