Skip to content
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

Merged
merged 14 commits into from
Apr 10, 2023

Conversation

rumpfc
Copy link
Contributor

@rumpfc rumpfc commented Apr 4, 2023

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 in heading are filtered out, and missing keys in data rows (according to heading) will produce an empty cell.

$table = new \CodeIgniter\View\Table();

$table->setHeading(['name' => 'Name', 'color' => 'Color', 'size' => 'Size'])
    ->setSyncRowKeysWithHeadingKeys(true)
    ->addRow(['color' => 'Blue', 'name' => 'Fred', 'size' => 'Small'])
    ->addRow(['size' => 'Large', 'age' => '24', 'name' => 'Mary'])
    ->addRow(['color' => 'Green']);

echo $table->generate();

This produces HTML table

<table border="0" cellpadding="4" cellspacing="0">
    <thead>
        <tr>
            <th>Name</th>
            <th>Color</th>
            <th>Size</th>
        </tr>
    </thead>
    <tbody>
        <tr>
            <td>Fred</td>
            <td>Blue</td>
            <td>Small</td>
        </tr>
        <tr>
            <td>Mary</td>
            <td></td>
            <td>Large</td>
        </tr>
        <tr>
            <td></td>
            <td>Green</td>
            <td></td>
        </tr>
    </tbody>
</table>

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added enhancement PRs that improve existing functionalities 4.4 labels Apr 5, 2023
Copy link
Member

@michalsn michalsn left a 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.

system/View/Table.php Outdated Show resolved Hide resolved
system/View/Table.php Outdated Show resolved Hide resolved
@kenjis kenjis changed the title Feature: HTML Table data keys synchronize order with Heading keys feat: HTML Table data keys synchronize order with Heading keys Apr 5, 2023
system/View/Table.php Outdated Show resolved Hide resolved
Copy link
Member

@kenjis kenjis left a 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

/**
* Order each inserted row by heading keys
*/
public bool $syncRowsWithHeading = false;
Copy link
Member

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?

Copy link
Member

Choose a reason for hiding this comment

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

I think not.

Copy link
Contributor Author

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

Copy link
Member

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...

Copy link
Member

Choose a reason for hiding this comment

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

@paulbalandan

  1. public for current consistency
  2. change only this property to protected or private

Copy link
Member

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.

tests/system/View/TableTest.php Outdated Show resolved Hide resolved
@rumpfc rumpfc requested a review from kenjis April 6, 2023 18:51
system/View/Table.php Outdated Show resolved Hide resolved
@@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- **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.

@rumpfc rumpfc requested a review from kenjis April 7, 2023 12:31
system/View/Table.php Outdated Show resolved Hide resolved
Copy link
Member

@kenjis kenjis left a 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.

@rumpfc rumpfc requested a review from kenjis April 8, 2023 07:10
Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

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

Thank you!

Copy link
Member

@michalsn michalsn left a 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.

@kenjis kenjis merged commit d073d41 into codeigniter4:4.4 Apr 10, 2023
@kenjis
Copy link
Member

kenjis commented Apr 10, 2023

@rumpfc Thank you!

@rumpfc rumpfc deleted the feature-view-table-order branch April 14, 2023 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.4 enhancement PRs that improve existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants