-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
feat: HTML Table data keys synchronize order with Heading keys #7409
Conversation
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 is a helpful feature. Cool!!!
I have some doubts about the variable/method names you chose - a little too long for my taste. But you don't have to change them right away. I'm looking for others' opinions on this.
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.
Thank you. This is a great enhancement!
Please add this new feature to the changelog
https://github.com/codeigniter4/CodeIgniter4/blob/4.4/user_guide_src/source/changelogs/v4.4.0.rst#others-1
and link to the description page so that more users will know about it.
See how to link: https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/documentation.rst#to-a-section
system/View/Table.php
Outdated
/** | ||
* Order each inserted row by heading keys | ||
*/ | ||
public bool $syncRowsWithHeading = false; |
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.
Is there a need to make this public?
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 think not.
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.
All other properties are public
, I will keep it for consistency
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.
All other properties seems to be wrong, but changing them is breaking changes...
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.
public
for current consistency- change only this property to
protected
orprivate
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 would like this new property to be private
to enforce API encapsulation. We can increase visibility later if needed.
For the others, we can retain them as public
to prevent breaking changes.
@@ -87,6 +87,7 @@ Others | |||
See :ref:`controller-default-method-fallback` for details. | |||
- **Filters:** Now you can use Filter Arguments with :ref:`$filters property <filters-filters-filter-arguments>`. | |||
- **Request:** Added ``IncomingRequest::setValidLocales()`` method to set valid locales. | |||
- **Table:** Addedd ``Table::setSyncRowsWithHeading()`` method to synchronize row columns with heading. See :ref:`table-sync-rows-with-headings` for details. |
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.
- **Table:** Addedd ``Table::setSyncRowsWithHeading()`` method to synchronize row columns with heading. See :ref:`table-sync-rows-with-headings` for details. | |
- **Table:** Added ``Table::setSyncRowsWithHeading()`` method to synchronize row columns with headings. See :ref:`table-sync-rows-with-headings` for details. |
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.
Thank you for updating.
I commented two minor things inline.
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.
Thank you!
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.
Nice work. Thank you.
@rumpfc Thank you! |
Description
Table
is an ideal builder class to turn data and query results into an HTML table. That safes a lot of time to write an own builder.However there is a weakness when using custom Heading and data (no query results): If the number of headings does not match the number of columns in each row, the table becomes unbalanced. Furthermore when fetching data from APIs or other data sources (i.e. in XML or JSON), data must be manually rearranged to match the correct positions.
This feature introduces method
Table::setSyncRowKeysWithHeadingKeys
to internally rearrange the positions to match a certain key defined in setHeading if used with an associative array. Data row keys not defined inheading
are filtered out, and missing keys in data rows (according toheading
) will produce an empty cell.This produces HTML table
Checklist: