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

Add an ONS table block #108

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Add an ONS table block #108

wants to merge 14 commits into from

Conversation

zerolab
Copy link
Contributor

@zerolab zerolab commented Feb 28, 2025

What is the context of this PR?

This PR adds:

  • a new table block using TinyMCE limited to the table plugin
  • an ONS Table Block with source/footer notes

Screen recording

To-Do:

  • integration test that adds a table and checks the output

How to review

Check the branch out, add/edit a statistical artcile/ methodology page.

Follow-up Actions

@zerolab zerolab requested a review from a team as a code owner February 28, 2025 09:31
@zerolab zerolab marked this pull request as draft February 28, 2025 09:31
@zerolab zerolab force-pushed the tableblock-CMS-79 branch 2 times, most recently from ce7c890 to f859592 Compare February 28, 2025 09:51
Copy link
Contributor

@MebinAbraham MebinAbraham left a comment

Choose a reason for hiding this comment

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

Done an initial pass, this looks really good. Open sourcing TinyTableBlock 🎉 👍

  • The Row header option does not work as expected when using the UI buttons. It works as expected when copying/pasting, but it does not "understand" that we have row headers and the icon is not highlighted. The column header is working as expected.
  • We should add functional tests to validate the table behaviours. I feel like there are a few gaps that we don't test. We should test the different functionality and conditional cases such as each element is rendered if input, such as title, caption etc.
  • There is an alignment issue with the table/page. It is indented too much. See attached image:
image

@zerolab
Copy link
Contributor Author

zerolab commented Mar 3, 2025

There is an alignment issue with the table/page. It is indented too much. See attached image

That is because I wrapped the whole output in a figure element. Will change to a div

@zerolab zerolab marked this pull request as ready for review March 3, 2025 09:27
@zerolab zerolab requested a review from MebinAbraham March 4, 2025 15:47
@zerolab
Copy link
Contributor Author

zerolab commented Mar 4, 2025

@MebinAbraham I've re-tagged you. Will shortly switch to using the PyPI package for TinyTableBlock, but I think the changes are worth checking here

Copy link
Contributor

@sanjeevz3009 sanjeevz3009 left a comment

Choose a reason for hiding this comment

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

Minor comments. LGTM! Will approve once the changes in progress are complete and I re-review =)

@zerolab zerolab requested a review from RealOrangeOne March 5, 2025 16:44
<caption class="ons-table__caption{{ " ons-u-vh" if params.hideCaption }}">{{ params.caption }}</caption>
{% endif %}
<thead class="ons-table__head">
{% set headers = params.headers if params.headers else parms.ths|list %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo, hides the invalid param use.

Suggested change
{% set headers = params.headers if params.headers else parms.ths|list %}
{% set headers = params.headers if params.headers else params.ths|list %}

@MebinAbraham
Copy link
Contributor

MebinAbraham commented Mar 6, 2025

  • The Row header option does not work as expected when using the UI buttons. It works as expected when copying/pasting, but it does not "understand" that we have row headers and the icon is not highlighted. The column header is working as expected.

This still seems to be outstanding.

Also, I feel like there are still gaps in the test. We should have confidence in the features TinyMCE provide such as merged cells, row headers, column headers etc. For headers, we have tests that validate the value is displayed but not whether it is syntactically correct. At the moment, we don't validate the rendered version for things like merged cells, row headers, column headers etc.

Other than some gaps in tests, LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants