-
Notifications
You must be signed in to change notification settings - Fork 0
share current page button #27
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
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
| private getCurrentPage() { | ||
| const sameUrls = this.archiveList | ||
| ?.getAllPages() | ||
| // @ts-expect-error - TS2339 - Property 'pageUrl' does not exist on type 'ArgoViewer'. | ||
| .filter((p) => p.url === this.pageUrl); | ||
| if (!sameUrls || !sameUrls.length) { | ||
| return null; | ||
| } | ||
|
|
||
| // Sort by timestamp (newest first) | ||
| return sameUrls.sort((a, b) => { | ||
| const tsA = parseInt(a.ts, 10); | ||
| const tsB = parseInt(b.ts, 10); | ||
| return tsB - tsA; // Descending order (newest first) | ||
| })[0]; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Add null check for this.archiveList before accessing getAllPages() to prevent potential runtime errors when archiveList is null or undefined. [possible issue, importance: 8]
| private getCurrentPage() { | |
| const sameUrls = this.archiveList | |
| ?.getAllPages() | |
| // @ts-expect-error - TS2339 - Property 'pageUrl' does not exist on type 'ArgoViewer'. | |
| .filter((p) => p.url === this.pageUrl); | |
| if (!sameUrls || !sameUrls.length) { | |
| return null; | |
| } | |
| // Sort by timestamp (newest first) | |
| return sameUrls.sort((a, b) => { | |
| const tsA = parseInt(a.ts, 10); | |
| const tsB = parseInt(b.ts, 10); | |
| return tsB - tsA; // Descending order (newest first) | |
| })[0]; | |
| } | |
| private getCurrentPage() { | |
| if (!this.archiveList) { | |
| return null; | |
| } | |
| const sameUrls = this.archiveList | |
| .getAllPages() | |
| // @ts-expect-error - TS2339 - Property 'pageUrl' does not exist on type 'ArgoViewer'. | |
| .filter((p) => p.url === this.pageUrl); | |
| if (!sameUrls || !sameUrls.length) { | |
| return null; | |
| } | |
| // Sort by timestamp (newest first) | |
| return sameUrls.sort((a, b) => { | |
| const tsA = parseInt(a.ts, 10); | |
| const tsB = parseInt(b.ts, 10); | |
| return tsB - tsA; // Descending order (newest first) | |
| })[0]; | |
| } |
| private async onShareCurrent() { | ||
| const currentPage = this.getCurrentPage?.() || null; | ||
| if (!currentPage) { | ||
| alert("No current page to share."); | ||
| return; | ||
| } | ||
| console.log("Current page to share:", currentPage); | ||
| await this.onShare([currentPage]); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Remove the optional chaining operator (?.) when calling this.getCurrentPage() since it's a defined method in the class and not an optional property. [general, importance: 6]
| private async onShareCurrent() { | |
| const currentPage = this.getCurrentPage?.() || null; | |
| if (!currentPage) { | |
| alert("No current page to share."); | |
| return; | |
| } | |
| console.log("Current page to share:", currentPage); | |
| await this.onShare([currentPage]); | |
| } | |
| private async onShareCurrent() { | |
| const currentPage = this.getCurrentPage() || null; | |
| if (!currentPage) { | |
| alert("No current page to share."); | |
| return; | |
| } | |
| console.log("Current page to share:", currentPage); | |
| await this.onShare([currentPage]); | |
| } |
Shrinks99
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested, looks good!
User description
closes #8
PR Type
Enhancement
Description
Add "Share Current Page" button
Display archiving status card with completion message
Add divider between status and content
Refactor sharing functionality for selected/current pages
Changes walkthrough 📝
argo-archive-list.ts
Add method to retrieve all pagessrc/argo-archive-list.ts
getAllPages()method to retrieve all pages from the archive listsidepanel.ts
Implement archiving status card with share buttonsrc/sidepanel.ts
getCurrentPage()method to find the current page in archiveselected/current pages