joomla-projects / gsoc21_frontend-inline-editing

GNU General Public License v2.0
7 stars 2 forks source link

Refactoring of the FE Inline Edition #5

Closed anibalsanchez closed 3 years ago

anibalsanchez commented 3 years ago

Problem identified

We have the code to do the FE Inline Edition of the article title, article text area and custom fields.

Now, we have to do the refactoring to move the code into traits and interfaces to make it re-usable and extensible at the CMS Level.

These would be the ideal cases for an extension developer

Case 1. I'm an extension developer of an extension. The extension has a form with several fields. One of the fields is a textarea. The extension has a form view, a controller and a model that saves the form.

I want to make the textarea editable.

I can add declarations to the controller and the view. The model already knows how to save the data, so nothing has to be added here. I don't want to add more methods in the controller or in the model or in the view for the text area inline edition.

Something like:

Class MyController {
    use SaveForTheFEInlineEdition;
}

Class TheModel {
   use SaveForFEInlinrEditonIncrementalItem;
}

Case 2. Additionally, the textarea has a custom validation. It has to start with a number.

Case 3. Finally, the custom form can have custom fields.

So, I would want to do:

Class MyController implements FEInlineController, FEInlineValidation {
    use SaveForTheFEInlineEdition;

    protected function validateFEInlineEdition(Request $request) {
        bla bla bla
    }
}

Class TheModel implements FEInlineModel, FEInlineCustomSave {
   use SaveForFEInlineEditonIncrementalItem;

    protected function saveFEInlineEdition(Model $item) {
        bla bla bla
    }   
}

The custom area field extended with the validation and the front end edition:

class PlgFieldsMyCustomTextarea implements FEInlineEdition, FEInlineEditionCustomRender extends ...PlgFieldsTextarea
{
    public function doTheCustomRender() {
        // Add a custom validation to be sure that people add the number in the first line
    }
}

Proposed solution

There two main cases:

  1. Named Field Case: the field is in the Model and we manage the named field as parameter
  1. Custom Field Case: the fields is a custom field. It has a plugin to support it.

The traits, interfaces and function names are only examples for educational purposes. Feel free to change them to follow a different convention.

Open questions

Can the current code be refactored into this organization?

CC @bembelimen @roland-d

roland-d commented 3 years ago

@anibalsanchez One issue I see with saving using the existing model is that if you have multiple required fields and we are only saving one field. This will fail the form validation. In addition there can be other checks as well which can prevent the inline field from being saved.

We need to think of something that will validate just that one field we are saving. Make sure that the field meets all the requirements for that field. How to deal with that?

I do like the idea of using traits and interfaces, as you said, we just need to come up with some logical names for them.

anibalsanchez commented 3 years ago

@roland-d I think that the save must be always via the model. Since it has to save one field, the incremental save must load the object, apply the field change and the save the full object. In this way, the object is validated against the full consistency in the context of the business model.

roland-d commented 3 years ago

Sounds good to me but we need to make sure models do this. Extension developers will need to do that for their own extensions if they support inline editing.

rs4231199 commented 3 years ago
  1. Named Field Case: the field is in the Model and we manage the named field as parameter HTMLHelper : : ('FEInlineEdition.text', ...); HTMLHelper : : ('FEInlineEdition.textarea', ...);

@anibalsanchez Could you explain the role of these HTMLHelper's methods?

The given below are the changes I performed in the view part to make the custom text fields inline editable. $canEdit(if condition) will be moved to HTMLHelper which will reduce the work in view.

diff --git a/components/com_fields/layouts/field/render.php b/components/com_fields/layouts/field/render.php
index 84b3cd6476..e466544b48 100644
--- a/components/com_fields/layouts/field/render.php
+++ b/components/com_fields/layouts/field/render.php
@@ -9,12 +9,14 @@
 defined('_JEXEC') or die;

 use Joomla\CMS\Language\Text;
+use Joomla\CMS\HTML\HTMLHelper;

 if (!array_key_exists('field', $displayData))
 {
    return;
 }

+$item = $displayData['item'];
 $field = $displayData['field'];
 $label = Text::_($field->label);
 $value = $field->value;
@@ -28,6 +30,16 @@
    return;
 }

+$canEdit = $item->params->get('access-edit');
+$dataAttributes = '';
+$inlineEditClass = '';
+
+if ($canEdit && $field->type == 'text')
+{
+   $dataAttributes = HTMLHelper::_('convertToDataAttributes', 'com_fields', 'field', [ 'item_id' => $item->id, 'field_id' => $field->id ]);
+   $inlineEditClass = 'inline-editable-text';
+}
+
 ?>
 <?php if ($showLabel == 1) : ?>
    <span class="field-label <?php echo $labelClass; ?>"><?php echo htmlentities($label, ENT_QUOTES | ENT_IGNORE, 'UTF-8'); ?>: </span>
@@ -35,7 +47,7 @@
 <?php if ($prefix) : ?>
    <span class="field-prefix"><?php echo htmlentities($prefix, ENT_QUOTES | ENT_IGNORE, 'UTF-8'); ?></span>
 <?php endif; ?>
-<span class="field-value"><?php echo $value; ?></span>
+<span class="field-value <?php echo $inlineEditClass ?>"  <?php echo $dataAttributes ?>><?php echo $value; ?></span>
 <?php if ($suffix) : ?>
    <span class="field-suffix"><?php echo htmlentities($suffix, ENT_QUOTES | ENT_IGNORE, 'UTF-8'); ?></span>
 <?php endif; ?>

Here, the work for the developer in the view part would be i) Pass following to the HTMLHelper

ii) echo the returned values as HTML attribute.

What all will be the parameters for your HTMLHelper methods? And will it use the data attributes?

anibalsanchez commented 3 years ago

Hi,

The convertToDataAttributes is a way to convert data attributes. It is going to work fine if you call it in the layout and render the tags as you wrote it.

However, the FE Inline Edition is not something that you want to resolve in a "layout". The layout shouldn't have logic beyond displaying the layout. The render has to only render. On Joomla, we have layouts that are written in PHP, so it is tempting to put everything in the layout. If we were coding on a system with Twig as the rendering engine, it would be much clearer how hard it is to put logic in the layout.

It is better if we avoid the logic to activate FE Inline Edition in the layout, it has to go somewhere else.

For instance, it is better if the textarea field auto-activates inline edition or it is better if a controller or a form manager activates the inline edition of the textarea. When the inline edition has to be activated, it calls the HTMLHelper : : _('FEInlineEdition.text', ...) or HTMLHelper : : _('FEInlineEdition.textarea', ...) with the params that you need to generate the data attributes (you can even have convertToDataAttributes as private function). This is one idea. If you have a better place to refactor the rendering code, feel free to put it in another helper.

anibalsanchez commented 3 years ago

I've created a new RFC #8. We are still discussing the details, but it will probably replace this RFC.

anibalsanchez commented 3 years ago

We are closing this PR and continuing discussing the refactoring in #8