[#12329] Refactoring of sortable tables - Sessions table

[Zxun2]

Part of #12329

Outline of Solution

Note: This refactoring works are only on the sessions-table component which affects Instructor Home and Instructor Sessions. It should be easy enough to integrate sessions-table later on to other sessions-related tables (e.g. recycle-bin-table).

Previously, the data on the table component relies on the information provided by x-tableRowModels -- meaning that angular will "capture" and "trigger" a UI change in the DOM should there be any changes to x-tableRowModels. The usage of the sortable-table component meant that rowData is now responsible for "triggering" this UI change, which results in an indirect dependency to x-tableRowModels because there is a need to sync x-tableRowModels when a change in rowData is detected, and vice versa. This dependency is likely to be justified, as this is a scalable way to ensure that the sortable-table component is as versatile as possible. However, more caution is needed when developing as the syncing of two states is very bug prone. E2E testing is very beneficial here.

Sortable Tables

The existing component is very well defined. However, there is currently no way to customize the stylings of the table, so I added that capability. Generally, there are two supported stylings (as i have observed):

style 1	style 2

I have also modified ColumnData to add two new attributes, alignment and headerClass.

• alignment denotes the text-position in the cell and supports the conventional, 'start' | 'center' | 'end'.
• headerClass is useful for any additional stylings.

/**
 * Column data for sortable table
 */
export interface ColumnData {
  :
  alignment?: 'start' | 'center' | 'end'; // defaults to start via getAlignment method
  headerClass?: string; // additional styles
}

and the componentData of SortableTableCellData is now a function that accepts an index, indicating it's row's position in the sortable-table.

componentData: (idx: number) => Record<string, any>, // @Input values for component

Additionally, the component now emits a sortEvent (whenever sortRows is called), which is required to synchronize the state of rowsData and x-tableRowModels.

Sessions Table

The entire sessions table is now completely replaced with the sortable-table component. To support the current stylings of sessions-table, I created the following components:

• cell-with-tooltip
• cell-with-group-buttons
• cell-with-response-rate

These are the two functions responsible for creating the rows and columns:

  /**
   * Creates the column data for the table.
   * 
   * @param config Contains the information to create a column data.
   * @returns An array containing a column's data for the table
   *  if the column is to be shown, otherwise an empty array.
   */
  createColumnData(config: SessionsTableColumnData): ColumnData[] {
    if (!(config?.columnType === undefined) && !this.columnsToShow.includes(config.columnType!)) {
      return [];
    }

    const columnData: ColumnData = {
      header: config.header,
      ...(config.sortBy && { sortBy: config.sortBy }),
      ...(config.headerToolTip && { headerToolTip: config.headerToolTip }),
      ...(config.alignment && { alignment: config.alignment }),
      ...(config.headerClass && { headerClass: config.headerClass }),
    };

    return [columnData];
  }

  /**
   * Creates the row data for the table.
   * 
   * @param config Contains the information to create a row data.
   * @returns An array containing a row's data for the table if the row is to be shown,
   * otherwise an empty array.
   */
  createRowData(config: SessionsTableRowData): SortableTableCellData[] {
    if (!(config?.columnType === undefined) && !this.columnsToShow.includes(config.columnType!)) {
      return [];
    }

    const rowData: SortableTableCellData = {
      ...(config.value && { value: config.value }),
      ...(config.displayValue && { displayValue: config.displayValue }),
      ...(config.customComponent && { customComponent: config.customComponent }),
      ...(config.style && { style: config.style }),
    };

    return [rowData];
  }

Screenshots

It looks exactly like it currently does...

[jasonqiu212]

Hi @Zxun2, Sorry for the delay! Will get to this PR within these couple of days

[weiquu]

Hi @Zxun2, can I check the rationale for changing the default sort behaviour for the Instructor Home Page to session name ascending? I would have thought sorting by session end date descending would be more useful (e.g. for courses with many sessions, I want to see sessions that are ongoing / coming up over sessions where the end date has passed)

Also noted for the instructor sessions page, the default behaviour was changed from descending to ascending - think this is fine!

Overall, everything looks great (: Currently working through the code itself - will update in a bit

Edit: I also noticed some changes to contribution-question-statistics.component.ts - are those changes relevant to the PR? Was a bit surprised to notice the file there

Edit 2: Have looked through the rest of the code, looks good! Will go through 1 more time over the weekend to double check

[Zxun2]

Hey @weiquu, thanks for the review! I can't recall the exact rationale, but it was along the lines of, if it was

• ASC, then sessions that are closed will be placed on top (because they have the earliest end date)
• DSC, then sessions with the latest ending date will be placed on top (i thought this didn't make much sense since i expect that sessions that are open and ending soon should be on top instead.)

so i settled on sorting by Session Name in the end 😅 I have reverted that change to the original (which is Session End Date DSC, and if necessary, we can implement the above in another PR.

I also noticed some changes to contribution-question-statistics.component.ts - are those changes relevant to the PR? Was a bit surprised to notice the file there

I changed the SortableTableCellData interface which necessitates changes to contribution-question-statistics.component.ts which is already using the sortable-table component. The componentData attribute was originally Record<string, any> but it is now a function which accepts an idx which denotes the row it belongs to.

edit: CI needs a re-run :)

[weiquu]

Thanks @Zxun2 for the change and the info! Just one small issue to fix (:

(Taking this opportunity to ping @jasonqiu212 @domlimm @cedricongjh @EuniceSim142 - if a couple of you could review this PR soon that would be great)

[weiquu]

LGTM! Thanks for the changes and the great work (:

[jasonqiu212]

Outstanding work @Zxun2! Thanks for the huge refactor :) Just 2 small questions

[jasonqiu212]

LGTM, thanks @Zxun2!