WIP: adds datatable to index datatype. #370
Conversation
| $id = $this->pageListNode->getAttribute('id'); | ||
| $date = $this->getSortDateTime(); | ||
| $dataMtx = [ | ||
| __(self::INDEX_TABLE_COLUMNS_TITLE) => [ |
There was a problem hiding this comment.
I wonder if using the translated titles as array indices here is the best approach? I would tend to use some kind of (untranslated) name as the index, and then put the translated title in a field next to class/sort/value?
| $date = $this->getSortDateTime(); | ||
| $dataMtx = [ | ||
| __(self::INDEX_TABLE_COLUMNS_TITLE) => [ | ||
| 'class' => 'type_'.get_class($this).' '.($this->privateFlag ? 'is-private' : 'is-public'), |
There was a problem hiding this comment.
These things (datatype and private flag) seem to be unrelated to the title, so I wonder if these are appropriate here? Conceptually, they would be a column of their own, but you probably do not want to display them as such.
Maybe these would be more appropriate as classes on the <tr> instead of an individual column? Maybe returned as a special column in this array?
| ROW; | ||
| $tRowItemTpl = <<<ROWITEM | ||
| <td class="index-item [[name]] [[class]]" data-sort="[[sort]]">[[value]]</td> | ||
| ROWITEM; |
There was a problem hiding this comment.
I'm not sure if I like using these HTML templates like this. They are easy to read, but building HTML from text also seems prone to typos, encoding errors, etc. For forms, we're using this approach because those are complex and very diverse, but here the structure is very repetitive and simple, which means it could be feasible to just build up the DOM in memory like we do elsewhere. This has the advantage that all encoding is done automatically, you can't break the DOM with a typo, you can easily make changes to earlier sections of the DOM, etc.
system/core/pages.php
Outdated
| 'value' => $date ? $date->format('Y-m-d') : '', | ||
| ], | ||
| ]; | ||
| return array_intersect_key($dataMtx, array_fill_keys(self::getIndexTableColumns(), '')); |
There was a problem hiding this comment.
What is the point of this array_intersect? This seems to drop any keys from $dataMtx that do not appear in getIndexTableColumns(), but in the code I can see that this does not happen? Or is the point that subclasses can delete columns from their superclasses by removing them from getIndexTableColumns (by overriding it)? If so, I'd suggest that those superclasses can also just delete the columns from getIndexData (by overriding that as well), or maybe index page should globally just do this intersect (rather than each datatype having to do it individually)?
| __(self::INDEX_TABLE_COLUMNS_TITLE) => [ | ||
| 'class' => 'type_'.get_class($this).' '.($this->privateFlag ? 'is-private' : 'is-public'), | ||
| 'sort' => preg_replace("/[^A-Za-z0-9]/", '', $this->getTitle()).'_'.$id, | ||
| 'value' => '<a href="'.$this->language.'/'.$this->pagename.'">'.$this->getTitle().'</a>', |
There was a problem hiding this comment.
Should this be html_value to distinguish between html and text value? Alternatively, the non-HTML values should be htmlspecialchars'd? Alternatively, this could maybe build a DomNode to indicate that it is already HTML (combined with my other suggestion to build a DOM tree instead of an HTML string), and then any strings can be treated as text (and thus encoded).
Still not ready to merge, but reviews are highly appreciated.