Skip to content

Provide links to related models from edit views (CO-2229) #20

Closed
wants to merge 4 commits into from

Conversation

arlen
Copy link
Contributor

@arlen arlen commented Oct 12, 2021

No description provided.

Copy link
Contributor

@Ioannis Ioannis left a comment

Choose a reason for hiding this comment

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

Some thoughts

* @license Apache License, Version 2.0 (http://www.apache.org/licenses/LICENSE-2.0)
*/

if($this->request->getParam('action') == 'edit') {
Copy link
Contributor

Choose a reason for hiding this comment

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

@arlen Would it help if the configuration was part of the Controller or part of a helper?

* @license Apache License, Version 2.0 (http://www.apache.org/licenses/LICENSE-2.0)
*/

if($this->request->getParam('action') == 'edit') {
Copy link
Contributor

Choose a reason for hiding this comment

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

@arlen Would it help if the configuration was part of the Controller or a Helper?

Copy link
Contributor

Choose a reason for hiding this comment

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

In the current codebase, we generally set topLinks via buttons.inc, meaning the configuration is in the view (template) directory close to the rest of the configuration, so I think defining template specific links in the templates directory makes sense. I'm not a big fan of fieldsConfig.inc as the name, since I don't know how to distinguish it from fields.inc. We could stick with buttons.inc, unless we want to merge in the search.inc files at some point.

The other option would be to switch fields.inc to an entirely configuration driven file, like columns.inc. So, for example, we'd rewrite something like

if($action == 'add' || $action == 'edit') {
  print $this->Field->control('name');
  print $this->Field->control('description', [], false);
}

to something like

$controls = [
 'name' => [
  'actions' => ['add', 'edit'],
  'required' => false
 ]
];

and then $topLinks could be defined in the same file. I'm not sure how this will work for javascript logic that we have to eventually insert (render field X when field Y is true), but maybe we want that to be configuration driven also so we don't have to copy and paste javascript all over the place.

BTW $this->request->getParam('action') could be simplified down to $action, which is set in add-edit-view.php.

Also, maybe we should rename $topLinks to something less dependent on the position.

Copy link
Contributor Author

@arlen arlen Oct 14, 2021

Choose a reason for hiding this comment

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

I like the idea of having the fields.inc file be entirely configuration driven. This would keep the number of files simple and clean, and (as you state) we just put the links configuration in fields.inc. This will make the approach consistent for both the edit and index views. Instead of "topLinks" we might do "primaryLinks" or "mainLinks" ...or, given the nature of the add-edit view, just "links" (since we have no other collection of its kind - and even if we did it could probably be extended from "links" rather than creating something new).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been thinking about a naming convention for this file, and perhaps simply nav.inc makes sense: it could be used for any navigation that's associated with the current view - whether subnavigation (i.e. tabs) or secondary links (topLinks). I think in the case of $topLinks, we can just call these $links. I've considered $primaryNav and $secondaryNav, but I'm not sure those names are explicit enough ... So I'm leaning towards $subnavigation and $links -- names which identify the relationship of the items but not the position or choice of widget.

@arlen arlen marked this pull request as draft October 22, 2021 16:04
@arlen
Copy link
Contributor Author

arlen commented Oct 22, 2021

This has been converted to a draft - the better UI/UX solution for the models we're discussing here is to use subnavigation tabs because the models are integral to each other - and should be presented as a "unit".

@arlen
Copy link
Contributor Author

arlen commented Oct 25, 2021

This has been rebased against the latest develop and completely refactored. We now make use of a nav.inc file to incorporate general links (topLinks) and subnavigation. The behavior for the UX is better. There are two ways this could be improved, though neither are blockers:

  1. Titles for related models that are connected by subnavigation could do a better job of reflecting that relationship, and

  2. the "links" (ne topLinks) are not currently allowed in the add-edit-view.php view. While we don't need them included there at the moment, we undoubtedly will in Registry PE, and this implies further refactoring (possibly into elements) for the titleNavContainer to further avoid duplicate code.

* @param string $modelName Model name for form
* @param string $action Current action
* @param string $editable True if controls are read/write, false for read only
* @param string $cssClasses CSS classes to add to the form list
* @return string
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only the last parameter is an addition - the other lines here have one space of added white space to line up the descriptions but are otherwise unchanged.

'mapping' => 'nicknames.en'
],
'class' => 'buildbutton'
]
];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"topLinks" are now included in the nav.inc file as "links".

],
'actions' => ['index','edit','add']
];
?>
Copy link
Contributor Author

@arlen arlen Oct 25, 2021

Choose a reason for hiding this comment

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

It's likely that we'll need to nest the actual links in the "links" array down a level much like the $subnavigation array so we can provide the same kind of meta information, e.g. what actions allow the rendering of the links (which is what "actions" is for in $subnavigation). This isn't needed now, but may be in time.

</script>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only change here is to drop the <script> tag to the bottom of the fields.inc file. Having this at the top can cause problems with CSS due to it's placement in the DOM. (Probably something to discuss at a dev meeting.)

</div>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The #topLinks block is moved into its own element.

$target
);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving this section up fixes breadcrumb layout issues for the config views.

@arlen arlen marked this pull request as ready for review October 25, 2021 20:12
$controller == 'AttributeMappings' ||
$controller == 'Rules' ||
$controller == 'RuleAttributes' ||
$controller == 'SystemsOfRecord')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this condition could become more readable if written like this:

        $controllers = [
          'MatchgridSettings',
          'Attributes',       
          'ApiUsers',         
          'AttributeGroups',  
          'AttributeMaps',    
          'AttributeMappings',
          'Rules',            
          'RuleAttributes',   
          'SystemsOfRecord'
        ];

if (!empty($vv_cur_mg) && in_array($controller, $controllers, true) {
... // Do stuff
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call - I'll update in this way (probably using $configControllers as name for clarity)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is made

@arlen
Copy link
Contributor Author

arlen commented Oct 26, 2021

This has been rebased against the latest develop.

@arlen arlen force-pushed the feature-co2229 branch 2 times, most recently from c7a60ef to 2d3978e Compare November 5, 2021 21:08
@arlen
Copy link
Contributor Author

arlen commented Nov 5, 2021

This has been rebased against the latest develop.

@arlen arlen marked this pull request as draft November 8, 2021 17:03
@arlen
Copy link
Contributor Author

arlen commented Nov 8, 2021

This PR has been converted to draft while we work on the edit views of related models within subnavigation.

@arlen arlen marked this pull request as ready for review November 15, 2021 03:02
@arlen
Copy link
Contributor Author

arlen commented Nov 15, 2021

This PR is ready for review. While this pass does not address title (and subtitle) handling, this is a big improvement in getting related models to feel like part of a cohesive whole.

This PR establishes subnavigation for related models, and can be seen when visiting Rule Attributes and Attribute Mappings.

@arlen arlen marked this pull request as draft January 10, 2023 14:55
@arlen
Copy link
Contributor Author

arlen commented Jan 10, 2023

This solution to CO-2229 is similar to but has been superseded by the approach to subnav tabs in Registry PE (in which a global subnavigation file holds the tabs for related models together in one place). We will rework this solution to be like PE.

@arlen arlen closed this Jan 10, 2023
@arlen arlen deleted the feature-co2229 branch January 10, 2023 14:57
Sign in to join this conversation on GitHub.
Labels
None yet
Projects
None yet
3 participants