WIP: adds datatable to index datatype. #370
No reviewers
Labels
No labels
Component: User interface
Component: Wymeditor
Help wanted
Level: Difficult
Level: Easy
Level: Moderate
Pagetype: Festival
Pagetype: Mailinglist
Pagetype: Peer reviewed article
Pagetype: Text
Privacy GDPR AVG
status: has conflicts
Status: Needs changes
Status: Needs discussion
Status: Needs review
Status: Ready to merge
Status: Waiting for response
Type: Bug
Type: Enhancement
Type: Question
Usecase: De Stadsbron
Usecase: Koppelting
Usecase: MeetjeStad
Value: Coders
Value: Security
Value: Users
Value: Visitors
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
harmen/hypha!370
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "hypha-117"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Still not ready to merge, but reviews are highly appreciated.
Would it make sense to add a prefix to these translation identifiers to indicate that they will be used as column headers?
Good point! Will do that!
Overall, looks like this is heading in the right direction. I added a bunch of inline comments.
I think that it would be good to split off the
getAuthor()changes into their own commit before finalizing this.What is the point of this array_intersect? This seems to drop any keys from
$dataMtxthat do not appear ingetIndexTableColumns(), 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 fromgetIndexTableColumns (by overriding it)? If so, I'd suggest that those superclasses can also just delete the columns fromgetIndexData(by overriding that as well), or maybe index page should globally just do this intersect (rather than each datatype having to do it individually)?Also, I think this is the first time we're using constants as translation string ids (instead of passing them to
__()literally). If we ever want to add some automatic translation checking, which I can imagine works by scanning the code for calls to__()and picking out the argument, this might make that more complicated.@ -61,0 +84,4 @@$id = $this->pageListNode->getAttribute('id');$date = $this->getSortDateTime();$data = [__(self::INDEX_TABLE_COLUMNS_TITLE) => [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?
@ -61,0 +85,4 @@$date = $this->getSortDateTime();$data = [__(self::INDEX_TABLE_COLUMNS_TITLE) => ['class' => 'type_'.get_class($this).' '.($this->privateFlag ? 'is-private' : 'is-public'),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?@ -61,0 +87,4 @@__(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>',Should this be
html_valueto distinguish between html and text value? Alternatively, the non-HTML values should behtmlspecialchars'd? Alternatively, this could maybe build aDomNodeto 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).If you just want the latest version, no need to keep a full array and sort that, just keep a latest version variable and whenever you find a later version, update that variable?
@ -41,1 +41,4 @@$this->html->linkScript('//cdn.datatables.net/1.10.22/js/jquery.dataTables.min.js');$this->html->linkStyle('//cdn.datatables.net/1.10.22/css/jquery.dataTables.min.css');Don't forget to add these locally later.
@ -42,0 +62,4 @@ROW;$tRowItemTpl = <<<ROWITEM<td class="index-item [[name]] [[class]]" data-sort="[[sort]]">[[value]]</td>ROWITEM;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.
@ -49,0 +74,4 @@$isInDataType = array_key_exists($page->getAttribute('type'), $dataTypes);if ($lang && $isPrivateOrUser && $isInDataType) {/** @var HyphaDatatypePage $hyphaPage */$hyphaPage = createPageInstance($this->O_O, $page);This should be
$isPublicOrUser(or$isNotPrivateOrIsUser).@ -49,0 +75,4 @@if ($lang && $isPrivateOrUser && $isInDataType) {/** @var HyphaDatatypePage $hyphaPage */$hyphaPage = createPageInstance($this->O_O, $page);$titleSort = preg_replace("/[^A-Za-z0-9]/", '', $hyphaPage->getTitle());Can this ever be false?
@ -49,2 +79,4 @@$titleSort .= '_'.$index;$pageList[$titleSort] = $hyphaPage;}}This duplicates the sort return in the column data, could the default sorting maybe reuse that (i.e. by using a user-defined sorting function after collecting all index data that looks at
title->sort?@ -65,2 +102,2 @@$capital++;$first = false;if (!is_array($item)) {$item = ['value' => $item];Should this list of columns be based on the datatypes available? That would mean you always get a "status" field, even for sites that do not have any articles at all (then the status column will be present but always empty). How about filling
$columnsbased on the result from allgetIndexDatacalls instead?@ -76,0 +143,4 @@$dateIndex = array_search(__(HyphaDatatypePage::INDEX_TABLE_COLUMNS_DATE), array_values($columns));$vars = ['sortIndex' => $dateIndex,Why build the header after the body?
$columnsis already know before right?What does this sortIndex mean exactly? Is this the default sorting (if so, then why sort the HTML at all)? Or the only option for interactive sorting? This could probably use a comment.
@ -49,0 +74,4 @@$isInDataType = array_key_exists($page->getAttribute('type'), $dataTypes);if ($lang && $isPrivateOrUser && $isInDataType) {/** @var HyphaDatatypePage $hyphaPage */$hyphaPage = createPageInstance($this->O_O, $page);Probably is
isPublicOrUsertrue
@ -49,0 +75,4 @@if ($lang && $isPrivateOrUser && $isInDataType) {/** @var HyphaDatatypePage $hyphaPage */$hyphaPage = createPageInstance($this->O_O, $page);$titleSort = preg_replace("/[^A-Za-z0-9]/", '', $hyphaPage->getTitle());I can think of ways it could be false. But I guess we can cover that case when it actually happens.
@ -41,1 +41,4 @@$this->html->linkScript('//cdn.datatables.net/1.10.22/js/jquery.dataTables.min.js');$this->html->linkStyle('//cdn.datatables.net/1.10.22/css/jquery.dataTables.min.css');done
@ -49,0 +75,4 @@if ($lang && $isPrivateOrUser && $isInDataType) {/** @var HyphaDatatypePage $hyphaPage */$hyphaPage = createPageInstance($this->O_O, $page);$titleSort = preg_replace("/[^A-Za-z0-9]/", '', $hyphaPage->getTitle());There is a page on De Stadsbron that has an incorrect type definition. So yes, it can be false.
@ -65,2 +102,2 @@$capital++;$first = false;if (!is_array($item)) {$item = ['value' => $item];I've fixed this by fetching the used datatypes when iterating over the pages.
@ -76,0 +143,4 @@$dateIndex = array_search(__(HyphaDatatypePage::INDEX_TABLE_COLUMNS_DATE), array_values($columns));$vars = ['sortIndex' => $dateIndex,No reason, I've moved this piece of code above the body.
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.Merge
Merge the changes and update on Forgejo.Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.