WIP: feat: add popup to recognize low-engagement comments#5
WIP: feat: add popup to recognize low-engagement comments#5devsdenepal wants to merge 7 commits intoaskbuddie:mainfrom
Conversation
|
|
||
| if (match) { | ||
| const commentCount = parseInt(match[1], 10); | ||
| if (commentCount < 5) { |
There was a problem hiding this comment.
Can you create a CONSTANT for this value like MINIMUM_COMMENTS & use it here
| if (commentCount < 5) { | |
| if (commentCount < MINIMUM_COMMENTS) { | |
| const runScriptButton = document.getElementById('runScript'); | ||
| const postList = document.getElementById('postList'); | ||
| const noPosts = document.getElementById('noPosts'); | ||
| if (runScriptButton) { |
There was a problem hiding this comment.
We can exit early here too.
if(!runScriptButton) {
console.error('#runScript button not found!');
return;
}
| postList.innerHTML = ''; | ||
| noPosts.style.display = 'none'; |
There was a problem hiding this comment.
It seems like this is required for clearing the existing content?
We can create a helper function like below also, can we use replaceChildren method rather than innerHTML?
function clearUI() {
postLisst.replaceChildren();
noPosts.style.display = 'none';
}There was a problem hiding this comment.
Thanks for the suggestion!
| (results) => { | ||
| if (chrome.runtime.lastError) { | ||
| console.error(chrome.runtime.lastError.message); | ||
| return; | ||
| } | ||
|
|
||
| const lowCommentPosts = results[0]?.result || []; | ||
| if (lowCommentPosts.length === 0) { | ||
| noPosts.style.display = 'block'; | ||
| } else { | ||
| lowCommentPosts.forEach((post) => { | ||
| const li = document.createElement('li'); | ||
| li.textContent = `Post #${post.posinset} - ${post.commentCount} comments`; | ||
| li.style.cursor = 'pointer'; | ||
|
|
||
| li.addEventListener('click', () => { | ||
| chrome.scripting.executeScript({ | ||
| target: { tabId: tab.id }, | ||
| func: leaveComment, | ||
| args: [post.posinset], | ||
| }); | ||
| }); | ||
|
|
||
| postList.appendChild(li); | ||
| }); | ||
| } | ||
| } | ||
| ); |
There was a problem hiding this comment.
This can be a separate function to make things more readable.
|
Overall, it looks good but can we please typescript here? After the build, the |
| "vite": "^6.0.5" | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
always add newline at the end of file.
If you are using vscode: https://stackoverflow.com/questions/44704968/visual-studio-code-insert-newline-at-the-end-of-files
| noPosts.style.display = 'none'; | ||
| const [tab] = await chrome.tabs.query({ active: true, currentWindow: true }); | ||
|
|
||
| if (tab.id) { |
There was a problem hiding this comment.
this if statement seems to be so complex.... try to reduce complexity for better maintainability and readability...
simple idea to reduce complexity is it extract this in separate function, and leverage early return.
+1 |
There was a problem hiding this comment.
try implementing this popup.js code as a react component. Since we’re using react, we can simplify all this complexities by leveraging react components to render the content as part of the react DOM.
Co-authored-by: Ashish Yadav <18111862+ashiishme@users.noreply.github.com>
3 participants
Description
Screenshot