Faulty list view when back navigating #1243

Arif-Khalid · 2024-02-15T12:26:19Z

Summary:

Changes Made:

New model table-settings which stores kept settings
New service issue-table-settings service which stores these table settings by table name
Synchronization of component table settings and service table settings
Clear of service table settings on routing
Settings saved include page index, page size, active sort and active sort direction

Proposed Commit Message:

Issue table settings such as page index are not 
saved when table is re-mounted.

This behavior inconveniences users as their settings 
are reset everytime they navigate to a specific issue and back.

Let's lift up the table settings of each mounted table to 
a service which the tables pull from when mounted.

codecov-commenter · 2024-02-15T12:28:42Z

Codecov Report

Attention: 23 lines in your changes are missing coverage. Please review.

Comparison is base (a9d38dd) 53.44% compared to head (63ddff1) 53.10%.

Files	Patch %	Lines
.../app/shared/issue-tables/issue-tables.component.ts	15.38%	8 Missing and 3 partials ⚠️
.../app/core/services/issue-table-settings.service.ts	27.27%	4 Missing and 4 partials ⚠️
src/app/core/models/table-settings.model.ts	0.00%	4 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1243      +/-   ##
==========================================
- Coverage   53.44%   53.10%   -0.35%     
==========================================
  Files         103      105       +2     
  Lines        2928     2951      +23     
  Branches      544      549       +5     
==========================================
+ Hits         1565     1567       +2     
- Misses       1014     1030      +16     
- Partials      349      354       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

nknguyenhc

Tested and it works well! Just some comment on code quality.

nknguyenhc · 2024-02-16T13:35:07Z

src/app/core/models/table-settings.model.ts

I suggest the member fields to have proper type annotation, so that the compiler can detect errors if the fields are not assigned the correct value. Also, the fields should have proper initial values.

Linter prevents me from adding type annotations to member fields that are initialized. Error: no-inferrable-types.
The initial values used are also what I deem proper as initially the tables are not sorted at all, internally matsort is initialised with an empty string which is what I am emulating here.

src/app/core/services/issue-table-settings.service.ts

nknguyenhc · 2024-02-16T13:54:47Z

src/app/shared/issue-tables/issue-tables.component.ts

+  sortChange(newSort: Sort) {
+    this.tableSettings.sortActiveId = newSort.active;
+    this.tableSettings.sortDirection = newSort.direction;
+    this.issueTableSettingsService.setTableSettings(this.table_name, this.tableSettings);
+  }
+
+  pageChange(pageEvent: PageEvent) {
+    this.tableSettings.pageSize = pageEvent.pageSize;
+    this.tableSettings.pageIndex = pageEvent.pageIndex;
+    this.issueTableSettingsService.setTableSettings(this.table_name, this.tableSettings);
+  }
+


I'm not sure if this is the best practice to sync settings of a table component with the service. I would suggest the following:

Service's settings values to be of type Observable<TableSettings>

Table component settings to sync with service's settings value using Observable::subscribe

In these methods, only call issueTableSettingsService::setTableSettings

@CATcher-org/2324s2 What do yall think?

In this case wouldn't I still need these two functions to update the TableSettings in the service.
I am not sure of the benefit of the observable implementation since the only things listening and emitting these changes is from the tables component.

i don't see the benefit for observables too. I think an observable implementation is beneficial if we have more than 1 table listening to the same set of settings but so far we are saving the settings for each table (or to be more precise table_name, but currently each table has a unique table_name).

cheehongw

LGTM

cheehongw · 2024-02-17T18:40:59Z

src/app/core/models/table-settings.model.ts

@@ -0,0 +1,6 @@
+export class TableSettings {
+  public sortActiveId = '';


mat-sort-active is a angular-material term and may not be immediately obvious what it might be

perhaps some inline documentation so it will be clearer that sortActiveId refers to the id of the column that the table is currently sorted by

nknguyenhc

LGTM

nknguyenhc · 2024-02-18T00:13:46Z

src/app/core/services/issue-table-settings.service.ts

+  private _tableSettingsMap: { [index: string]: TableSettings } = {};
+
+  public getTableSettings(tableName: string): TableSettings {
+    console.log(this._tableSettingsMap);


Do remember to remove this console.log line!

* Fix broken duplicate link (#1233) Fix the broken link of a duplicate issue Currently, the user cannot open the link to a duplicate issue when opening an issue, as described in #1228. The links now work as expected. * Add whitespace validation (#1237) * Add whitespace validation * Update whitespace validation for new issue * Update whitespace validation for title of new issues * Update whitespace validation for title of new issues * Move validators into core * Update import order --------- Co-authored-by: Misra Aditya <[email protected]> * Fix uncaught errors when attempting to access an invalid route There is an uncaught error when the users click on an invalid internal link in Markdown or enter an invalid link in browser. Internal links are unlikely to be used for bug reporting and are more likely to be invalid. Let's show an error toaster and stop the navigation when clicking on an internal link in Markdown. Also, redirect the users to the login page if the users enter invalid link in browser. * Set default branch to `main` Previously, image uploads depend on the user's default branch. Now, we set the branch for image upload to be `main`. Images will be uploaded to `main` as a result. --------- Co-authored-by: Chee Hong <[email protected]> * Preserve linebreaks (#1241) With preserving linebreaks, subset list items are rendered as a list, and paragraph rendering is the same as Github. * Faulty list view when back navigating (#1243) Issue table settings such as page index are not saved when table is re-mounted. This behavior inconveniences users as their settings are reset everytime they navigate to a specific issue and back. Let's lift up the table settings of each mounted table to a service which the tables pull from when mounted. * Upgrade to Angular 12 (#1242) Some of our packages are old and outdated. We should actively maintain and keep these packages up-to-date so it is easier to maintain in the future. Let's upgrade to Angular 12 to keep our packages up-to-date. * Fix markdown blockquote preview difference (#1245) Due to DOMPurify, the content used for preview is different. However, given that ngx-markdown already has sufficient sanitation by default, we remove sanitation by DOMPurify. * Create release for v3.5.3 --------- Co-authored-by: Nguyen <[email protected]> Co-authored-by: AdityaMisra <[email protected]> Co-authored-by: Misra Aditya <[email protected]> Co-authored-by: NereusWB922 <[email protected]> Co-authored-by: Chee Hong <[email protected]> Co-authored-by: Arif Khalid <[email protected]>

Arif-Khalid added 5 commits February 15, 2024 19:04

Save issue table settings

5936d1c

Fix import order

391bd7b

Add multiple settings for multiple tables

fbc5bc1

Fix import order

7ec259d

Document new service

67c2c85

Arif-Khalid added aspect-UIX category.Bug category.Enhancement an enhancement to an existing feature labels Feb 15, 2024

Arif-Khalid self-assigned this Feb 15, 2024

Remove unused import

e6f3b8e

Arif-Khalid requested review from cheehongw and vigneshsankariyer1234567890 February 15, 2024 12:30

nknguyenhc requested changes Feb 16, 2024

View reviewed changes

Arif-Khalid added 2 commits February 18, 2024 01:23

Add type annotation

511c4f5

Add return type annotation for methods

fd7882d

cheehongw approved these changes Feb 17, 2024

View reviewed changes

cheehongw reviewed Feb 17, 2024

View reviewed changes

nknguyenhc approved these changes Feb 18, 2024

View reviewed changes

Add comments

63ddff1

Arif-Khalid merged commit a1b5000 into CATcher-org:master Feb 19, 2024
4 of 5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Faulty list view when back navigating #1243

Faulty list view when back navigating #1243

Arif-Khalid commented Feb 15, 2024

codecov-commenter commented Feb 15, 2024 •

edited

Loading

nknguyenhc left a comment

nknguyenhc Feb 16, 2024

Arif-Khalid Feb 17, 2024

nknguyenhc Feb 16, 2024

Arif-Khalid Feb 17, 2024

cheehongw Feb 17, 2024

cheehongw left a comment

cheehongw Feb 17, 2024 •

edited

Loading

nknguyenhc left a comment

nknguyenhc Feb 18, 2024

		@@ -0,0 +1,6 @@
		export class TableSettings {
		public sortActiveId = '';

Faulty list view when back navigating #1243

Faulty list view when back navigating #1243

Conversation

Arif-Khalid commented Feb 15, 2024

Summary:

Changes Made:

Proposed Commit Message:

codecov-commenter commented Feb 15, 2024 • edited Loading

Codecov Report

nknguyenhc left a comment

Choose a reason for hiding this comment

nknguyenhc Feb 16, 2024

Choose a reason for hiding this comment

Arif-Khalid Feb 17, 2024

Choose a reason for hiding this comment

nknguyenhc Feb 16, 2024

Choose a reason for hiding this comment

Arif-Khalid Feb 17, 2024

Choose a reason for hiding this comment

cheehongw Feb 17, 2024

Choose a reason for hiding this comment

cheehongw left a comment

Choose a reason for hiding this comment

cheehongw Feb 17, 2024 • edited Loading

Choose a reason for hiding this comment

nknguyenhc left a comment

Choose a reason for hiding this comment

nknguyenhc Feb 18, 2024

Choose a reason for hiding this comment

codecov-commenter commented Feb 15, 2024 •

edited

Loading

cheehongw Feb 17, 2024 •

edited

Loading