Skip to content
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

Fix PR #329 #494

Merged
merged 18 commits into from
Jul 6, 2024
Merged

Fix PR #329 #494

merged 18 commits into from
Jul 6, 2024

Conversation

Jonghakseo
Copy link
Owner

@Jonghakseo Jonghakseo commented Jun 16, 2024

@Jonghakseo Jonghakseo changed the title PR #329 FIx PR #329 Jun 16, 2024
@Jonghakseo Jonghakseo changed the title FIx PR #329 Fix PR #329 Jun 16, 2024
src/pages/injectedContent/ui/index.tsx Outdated Show resolved Hide resolved
src/pages/injectedContent/ui/index.tsx Outdated Show resolved Hide resolved
src/pages/popup/Popup.tsx Outdated Show resolved Hide resolved
vite.config.ts Outdated Show resolved Hide resolved
@PatrykKuniczak PatrykKuniczak linked an issue Jun 16, 2024 that may be closed by this pull request
@PatrykKuniczak
Copy link
Collaborator

@Jonghakseo Are you finish this in the near future?

@Jonghakseo
Copy link
Owner Author

Jonghakseo commented Jun 17, 2024

@Jonghakseo Are you finish this in the near future?

Yep. I'm gonna finish this PR after current issue resolved.

@Jonghakseo Jonghakseo marked this pull request as ready for review June 17, 2024 15:28
Copy link
Collaborator

@PatrykKuniczak PatrykKuniczak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If user try to inject it to the place where he can't e.g extension store page/start page, error occur:
|image

I think good idea is handle that and create e.g console.error or sth like that for our purpose, then every dev can handle it in they own way, but when they see our exception handling in code, they probably don't forget to handle that case :)

src/pages/content/runtime/injected.css Outdated Show resolved Hide resolved
@PatrykKuniczak
Copy link
Collaborator

@Jonghakseo Let's slowly end this looong thread 😁

@Jonghakseo Jonghakseo requested a review from PatrykKuniczak July 6, 2024 07:17
@Jonghakseo
Copy link
Owner Author

If user try to inject it to the place where he can't e.g extension store page/start page, error occur:
I think good idea is handle that and create e.g console.error or sth like that for our purpose, then every dev can handle it in they own way, but when they see our exception handling in code, they probably don't forget to handle that case :)

IMO, each error message seems to clearly indicate what the problem is, and realistically, there may not be a single cause for every error case. Therefore, I think it is best to leave error handling to the user's responsibility.

@Jonghakseo Jonghakseo merged commit 3c9ccd4 into legacy Jul 6, 2024
4 checks passed
@Jonghakseo Jonghakseo deleted the inject-content-script branch July 6, 2024 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't add a custom Content Script folder
4 participants