Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
WIP: adds datatable to index datatype. #370
base: master
Are you sure you want to change the base?
WIP: adds datatable to index datatype. #370
Changes from 1 commit
8b8e0bb
6b74001
0593b43
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be
html_value
to distinguish between html and text value? Alternatively, the non-HTML values should behtmlspecialchars
'd? Alternatively, this could maybe build aDomNode
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).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the point of this array_intersect? This seems to drop any keys from
$dataMtx
that 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)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.