Closed wesrowe closed 1 year ago
laflannery
commented
2 years ago @wesrowe I worked with Val to improve the inline guidance for both the Table title and the Table headers. I was able to update the Figma file and also added a new screenshot below:
Let me know if there are any questions or you need anything else from me prior to implementation.
[EDIT: Wes replaced image in main ticket description]
EWashb
commented
2 years ago For the horizontal/vertical checkboxes, are those just to add headers? The labels could be more descriptive.
chri5tia
commented
2 years ago @EWashb It just defines if the first row or first column is the header. The header data would still be entered or already entered into the table as per prior to any of these proposed changes.
chri5tia
commented
2 years ago I need a sanity check. After messing around a little in the db, I switched to the main branch where the patch is not applied, ran composer install and database updates, re-logged in to my local instance and I still see the row and column header fields. For fun I also re-imported configs with drush cim. When that didn't work, I pulled in the database from scratch! Must be something with sequalace. I clicked refresh tables, rebuild, etc.
Sticking to what I know, (ruling out issues with using wysiwyg software) here is the db in the project branch:
MariaDB [db]> show columns in paragraph__field_table;
+---------------------------+------------------+------+-----+---------+-------+
| Field | Type | Null | Key | Default | Extra |
+---------------------------+------------------+------+-----+---------+-------+
| bundle | varchar(128) | NO | MUL | | |
| deleted | tinyint(4) | NO | PRI | 0 | |
| entity_id | int(10) unsigned | NO | PRI | NULL | |
| revision_id | int(10) unsigned | NO | MUL | NULL | |
| langcode | varchar(32) | NO | PRI | | |
| delta | int(10) unsigned | NO | PRI | NULL | |
| field_table_value | longblob | YES | | NULL | |
| field_table_format | varchar(255) | YES | | NULL | |
| field_table_caption | varchar(255) | YES | | NULL | |
| field_table_row_header | tinyint(4) | NO | | 0 | |
| field_table_column_header | tinyint(4) | NO | | 0 | |
+---------------------------+------------------+------+-----+---------+-------+Main branch:
MariaDB [db]> show columns in paragraph__field_table;
+---------------------------+------------------+------+-----+---------+-------+
| Field | Type | Null | Key | Default | Extra |
+---------------------------+------------------+------+-----+---------+-------+
| bundle | varchar(128) | NO | MUL | | |
| deleted | tinyint(4) | NO | PRI | 0 | |
| entity_id | int(10) unsigned | NO | PRI | NULL | |
| revision_id | int(10) unsigned | NO | MUL | NULL | |
| langcode | varchar(32) | NO | PRI | | |
| delta | int(10) unsigned | NO | PRI | NULL | |
| field_table_value | longblob | YES | | NULL | |
| field_table_format | varchar(255) | YES | | NULL | |
| field_table_caption | varchar(255) | YES | | NULL | |
| field_table_row_header | tinyint(4) | NO | | 0 | |
| field_table_column_header | tinyint(4) | NO | | 0 | |
+---------------------------+------------------+------+-----+---------+-------+
11 rows in set (0.009 sec)Can someone run ddev sqlc off the main branch and then run the sql command above to see if they get the same results? @dsasser @swirtSJW
field_table_column_header and field_table_column_row shouldn't be there if the patch adds this feature in the first place... right?
dsasser
commented
2 years ago I need a sanity check. After messing around a little in the db, I switched to the main branch where the patch is not applied, ran composer install and database updates, re-logged in to my local instance and I still see the row and column header fields.
@chri5tia This is expected. Running updates is a one way thing, you can't undo an update by running updates again. To undo the tablefield patch changes, you need to reset your db ddev pull va --skip-files
chri5tia
commented
2 years ago Thanks for the sanity check. I am not sure what occurred after I did the db pull before, but it's good now.
MariaDB [db]> show columns in paragraph__field_table;
+---------------------+------------------+------+-----+---------+-------+
| Field | Type | Null | Key | Default | Extra |
+---------------------+------------------+------+-----+---------+-------+
| bundle | varchar(128) | NO | MUL | | |
| deleted | tinyint(4) | NO | PRI | 0 | |
| entity_id | int(10) unsigned | NO | PRI | NULL | |
| revision_id | int(10) unsigned | NO | MUL | NULL | |
| langcode | varchar(32) | NO | PRI | | |
| delta | int(10) unsigned | NO | PRI | NULL | |
| field_table_value | longblob | YES | | NULL | |
| field_table_format | varchar(255) | YES | | NULL | |
| field_table_caption | varchar(255) | YES | | NULL | |
+---------------------+------------------+------+-----+---------+-------+Fighting a lot with this. Getting a lot of conflicts trying to rebase even after doing a git reset --hard head to remove all of my work and commits. Need to create a new project branch and close the existing draft PR.
Wondering if the tablefield module still holds enough value instead of other methods for table-style content creation: a) paragraph template for custom paragraph table type b) wysiwyg table insert for rich text c) https://www.drupal.org/project/paragraphs_table
In philosophy, there is a lot of practical value in not trying to reinvent the wheel or change things up to drastically, improving on existing software and methods, but at some point we may need to evaluate how much we've evolved away from tablefield continuing to be viable, considering all the patches we've already applied to it.
Will definitely need to rope in more minds to move this forward.
Other observations about the patch and/or the tablefield module:
Here is my draft patch for collab: https://github.com/chri5tia/patchwork/blob/main/patches/tablefield.patch
wesrowe
commented
2 years ago @laflannery, does 508 compliance require every table to have at least one header (whether it's at the top or left)? Val raised the question, as it would affect whether the new checkboxes are required. It would also affect whether we would continue to have the FE default to top-row header.
UPDATE: note, the Editor UI says that the Editor can leave the first row blank if they don't want the top row to be displayed as a header.
chri5tia
commented
2 years ago @dsasser Putting this here for now while we digest things: https://github.com/chri5tia/patchwork/blob/main/patches/tablefield1.patch These are the changes to move the form elements to the widget.
chri5tia
commented
2 years ago For collab purposes, as @dsasser and I work through troubleshooting, fixing and building out this feature, I am keeping track of things we've tried that don't necessarily relate to each other as separate scrap patches: https://github.com/chri5tia/patchwork/tree/main/patches
laflannery
commented
2 years ago @wesrowe Data tables should always have at least one header specified. Layout/presentation tables (tables used only to organize content into columns) do not need these headers. However, I do not believe this table widget is intended for layout purposes so I do not believe that case applies here. That case would also require additional markup if it does in fact apply, we would need to add some additional attributes. I had noticed that the current editor UI had the note about leaving the row blank, but I did not see this in the Figma screenshot so I thought that functionality/guidance it was being removed with this new patch.
Regarding defaults, during one meeting it was discussed how to handle defaulting the headers and I believe we had decided not to default within the UI but instead to create a FE template for top-row in order to assist with the QA/fixing of the tables. That way once all existing table were updated correctly, this default template could be removed and all new tables being created would have to select a header choice or it would not save properly.
jilladams
commented
2 years ago Per discussion with @wesrowe : this issue is blocked by a deeper design think & content audit in #10793. More context on the why forthcoming. Meantime: I've asked Daniel to update ticket so we can pause work for now and make it easy to resume later.
dsasser
commented
2 years ago We have a va.gov-cms PR that includes: adding the updated tablefield patch to composer, the update hook to solve for field storage definition updates, and the bugfix in va_gov_backend. This PR was, at the time, a complete solution to this issue. However, it was not long after the PR was opened that we were notified a design review process was going to kick off--and thus a pause on this issue.
The updated tablefield patch is an iteration of the original community provided patch, but the implementation approach could use some additional review. The community appears to be bisected on the path for the D8 version of tablefield, as it pertains to adding display options to the table field. One approach, the one we took, was to include the heading orientation as a pair of new boolean fields, presented as checkboxes on the table edit form. The other approach is one worth looking into however, as it utilizes a single database field to represent the heading choice. A single field means a smoother UX (fewer input elements) and less custom code (we wouldn't need to figure out how to "require" a checkbox without making it always checked--an implied requirement from the original design).
Further, consider this comment from the module maintainer, which suggests a sub-module for moving to entity-level override-able storage options for all tablefield formatters.
jilladams
commented
2 years ago Note: Swirt has specifically suggested (somewhere in Slack?) that we not go the submodule route.
jilladams
commented
1 year ago
Description
In https://github.com/department-of-veterans-affairs/va.gov-cms/issues/10379 we found that the tablefield patch mostly worked, but a bug prevented the Editor from saving work.
Designs, per #10674. (updated 9/13 per Laura's comment below)
Acceptance Criteria
CMS Team
Please check the team(s) that will do this work.
ProgramPlatform CMS TeamSitewide Crew⭐️ Sitewide CMS⭐️ Public Websites⭐️ Facilities⭐️ User support