-
Notifications
You must be signed in to change notification settings - Fork 0
feat!: rewrite #4
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
base: main
Are you sure you want to change the base?
Conversation
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.
a few things that jumped out to me
.gitignore
Outdated
*.sw? | ||
*.sw? |
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.
keep newline at the end of the file
eslint.config.js
Outdated
'@typescript-eslint/explicit-function-return-type': [ | ||
'error', | ||
{ | ||
allowExpressions: true, | ||
allowTypedFunctionExpressions: true, | ||
}, | ||
], | ||
// '@typescript-eslint/explicit-function-return-type': [ | ||
// 'error', | ||
// { | ||
// allowExpressions: true, | ||
// allowTypedFunctionExpressions: true, | ||
// }, | ||
// ], |
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.
why comment out when you could cut
package.json
Outdated
"name": "ut-lectures-plus", | ||
"description": "This plugin adds a transcript box (with search) to all LectureOnline and TowerLA lectures from UT Austin! Studying has never been easier.", | ||
"name": "wxt-solid-starter", | ||
"description": "manifest.json description", | ||
"private": true, | ||
"version": "5.0.0", | ||
"homepage": "https://github.com/Longhorn-Developers/UT-Lectures-Plus", | ||
"license": "MIT", | ||
"version": "0.0.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.
some of these things shouldn't've been changed
also, make sure the version is a major version bump from whatever was there previously
import { defineConfig } from 'wxt'; | ||
|
||
// See https://wxt.dev/api/config.html | ||
export default defineConfig({ | ||
srcDir: 'src', |
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.
feels weird to not have a src directory, floods the upper level
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.
Agreed, I also prefer having a src dir especially as we begin to populate this with dotfiles.
modules: ['@wxt-dev/module-solid'], | ||
imports: false, // Disable auto-imports |
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.
do we really want autoimports?
wxt.config.ts
Outdated
permissions: ['webRequest', 'tabs'], | ||
name: 'UT Lectures Plus', | ||
description: 'Enhance your UT Austin Lectures experience.', | ||
permissions: ['webRequest', 'tabs', 'storage'], |
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.
tabs is a very powerful persmission, i think it will pop up with "read and modify data on all sites", do we really need the tabs permission?
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.
We request the tabs
permission because the extension needs to handle scenarios where users have multiple lecture video tabs open. Specifically, the background script uses the tabs
API to:
- Track which transcript corresponds to which tab
- Sends transcripts to the correct active tab for display
You can see how this is being implemented here. If there's a better way to achieve this without using the tabs permission, I'd be happy to update it
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.
actually nvm, figured it out lol
I've updated the PR title to indicate that it contains breaking changes. |
@EthanL06 For such a big PR such as thsi one where you're rewriting most if not all of the codebase, please update your original comment to include the tech stack used and any other major changes you've made please. |
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.
Please refer to my previous comments in addition to updating README.md
.
I will be performing a more detailed PR review soon.
setTimeout(() => { | ||
const container = document.querySelector('.video-js') as HTMLElement; | ||
if (container) { | ||
const video = container.querySelector('video'); | ||
|
||
if (!video) { | ||
console.error('Video element not found'); | ||
return; | ||
} | ||
|
||
console.warn('🎥 Video found on fallback!'); | ||
observer.disconnect(); | ||
|
||
container.style.borderRadius = '1rem'; | ||
container.style.backgroundColor = 'oklch(98.4% 0.003 247.858)'; | ||
container.style.overflow = 'hidden'; | ||
|
||
video.style.borderRadius = '1rem'; | ||
|
||
setVideoElement(video); | ||
} | ||
}, 5000); // fallback after 5 seconds |
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.
You may want to clear this timeout if the mutation observer finds the video before the 5 seconds goes off
container.style.borderRadius = '1rem'; | ||
container.style.backgroundColor = 'oklch(98.4% 0.003 247.858)'; | ||
container.style.overflow = 'hidden'; | ||
|
||
video.style.borderRadius = '1rem'; |
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.
The style stuff here is duplicated above... also, in this section you don't set the videoContainer.style.boxShadow
, but you do above. Having a single function used in both places would reduce the duplication and the risk of missing a change in one place
…ound script with tabs permission
"homepage": "https://github.com/Longhorn-Developers/UT-Lectures-Plus", | ||
"license": "MIT", |
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.
I would add this back in
|
||
const addVTTListener = () => { | ||
if (!isListenerActive) { | ||
browser.webRequest.onBeforeRequest.addListener(requestListener, { urls: ['*://*.la.utexas.edu/*'] }, [ |
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.
Not a big fan of hardcoding this in. Let's extract this into a const object.
manifest: { | ||
permissions: ['webRequest', 'tabs'], | ||
name: 'UT Lectures Plus', | ||
description: | ||
'This plugin adds a transcript box (with search) to all LectureOnline and TowerLA lectures from UT Austin! Studying has never been easier.', |
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.
Would be a good idea to import this from package.json
@EthanL06 , just left some nit comments but we can fix the nit things now or in the future as they don't affect functionality, up to you. Overall great job with this rewrite! @Samathingamajig this PR LGTM once the comments/feedback have been addressed so that we can start creating issues and PRs that depend on this PR. |
Todo:
|
Entire rewrite of UTLP codebase.
Tech stack:
Major changes:
All of the main code is under
src/entrypoints
.src/entrypoints/main.content
: contains everything related to the sidebar.src/entrypoints/injected.content
contains everything related to the injected CSS on the website.src/entrypoints/popup
contains everything related to popupsrc/entrypoints/background.ts
is the background script