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

[4.0] Split the rules field to 2 layouts #19507

Merged
merged 15 commits into from
Feb 4, 2018
Merged

[4.0] Split the rules field to 2 layouts #19507

merged 15 commits into from
Feb 4, 2018

Conversation

anuragteapot
Copy link
Contributor

Summary of Changes

Splits the HTML part (JLayout) into two files.
Reasons:
1: less code per file
2: more efficient caching (less allocate memory, etc)

Testing Instructions

1: Apply patch
2: Go to Admin->Global Configuration
3: Check Permissions working as before

Expected result

This is code improvement

Actual result

Same as before

Documentation Changes Required

Yes

@anuragteapot
Copy link
Contributor Author

Removed ACL tooltips due to remove in base branch.

@wilsonge wilsonge merged commit 466566b into joomla:4.0-dev Feb 4, 2018
@wilsonge
Copy link
Contributor

wilsonge commented Feb 4, 2018

Thanks!

@wilsonge wilsonge added this to the Joomla 4.0 milestone Feb 4, 2018
@anuragteapot
Copy link
Contributor Author

anuragteapot commented Feb 5, 2018

@wilsonge Thanks :)

rdeutz added a commit that referenced this pull request Feb 5, 2018
because of #19507
@Bakual
Copy link
Contributor

Bakual commented Feb 5, 2018

Why was that Jlayout placed into layouts/joomla/form/field/rules/tabs.php and not like most other field layouts into layouts/joomla/form/field/rules.php.
Imho it looks like it was originally planned to have multiple layouts but in the end it was just one.

@anuragteapot
Copy link
Contributor Author

@Bakual yeah, you are right.
I have planned for two but end up one :p

@Bakual
Copy link
Contributor

Bakual commented Feb 5, 2018

Thanks. I'll move it then in my PR 😄

@anuragteapot
Copy link
Contributor Author

@Bakual Thanks

* @var boolean $hasValue Has this field a value assigned?
* @var array $options Options available for this field.
*
* Calendar Specific
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make a PR to correct the variables used here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should I remove JLayout variables which were not used in this file?

* Name of the layout being used to render the field
*
* @var string
* @since 3.5
Copy link
Contributor

Choose a reason for hiding this comment

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

since not 3.5 but __DEPLOY_VERSION__

*
* @return array
*
* @since 3.5
Copy link
Contributor

Choose a reason for hiding this comment

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

since not 3.5 but __DEPLOY_VERSION__

'assetRules' => $this->assetRules,
'isGlobalConfig' => $this->isGlobalConfig,
'parentAssetId' => $this->parentAssetId,
);
Copy link
Contributor

Choose a reason for hiding this comment

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

All these variables need to be explained in the doc block of the JLayout

@dgrammatiko
Copy link
Contributor

@Anu1601CS Please check my comments here and please make a PR to correct the doc blocks. Thanks

@anuragteapot
Copy link
Contributor Author

@dgt41 Thanks for review PR coming soon.

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.

None yet

5 participants