Skip to content

feat: HTML Table data keys synchronize order with Heading keys - #7409

Merged
kenjis merged 14 commits into
codeigniter4:4.4from
rumpfc:feature-view-table-order
Apr 10, 2023
Merged

kenjis merged 14 commits into
codeigniter4:4.4from
rumpfc:feature-view-table-order

Conversation

@rumpfc

@rumpfc rumpfc commented Apr 4, 2023

Copy link
Copy Markdown
Contributor

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

Comment thread user_guide_src/source/outgoing/table/020.php Outdated
@kenjis kenjis added enhancement PRs that improve existing functionalities 4.4 labels Apr 5, 2023

@michalsn michalsn left a comment

Copy link
Copy Markdown
Member

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.

Comment thread system/View/Table.php Outdated
Comment thread system/View/Table.php Outdated
@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
Comment thread system/View/Table.php
Comment thread system/View/Table.php Outdated
Comment thread tests/system/View/TableTest.php Outdated
Comment thread tests/system/View/TableTest.php Outdated
Comment thread user_guide_src/source/outgoing/table.rst Outdated
Comment thread user_guide_src/source/outgoing/table.rst Outdated
Comment thread user_guide_src/source/outgoing/table.rst Outdated
Comment thread user_guide_src/source/outgoing/table.rst Outdated

@kenjis kenjis left a comment

Copy link
Copy Markdown
Member

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

Comment thread system/View/Table.php Outdated
/**
* Order each inserted row by heading keys
*/
public bool $syncRowsWithHeading = false;

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread tests/system/View/TableTest.php Outdated
@rumpfc
rumpfc requested a review from kenjis April 6, 2023 18:51
Comment thread system/View/Table.php Outdated
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
Copy Markdown
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
Comment thread system/View/Table.php Outdated
Comment thread user_guide_src/source/changelogs/v4.4.0.rst Outdated

@kenjis kenjis left a comment

Copy link
Copy Markdown
Member

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

@kenjis kenjis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you!

@michalsn michalsn left a comment

Copy link
Copy Markdown
Member

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

kenjis commented Apr 10, 2023

Copy link
Copy Markdown
Member

@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

enhancement PRs that improve existing functionalities

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants