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

feat: add options page #158

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open

feat: add options page #158

wants to merge 24 commits into from

Conversation

ayoung19
Copy link
Contributor

@ayoung19 ayoung19 commented Jul 19, 2023

TODO:

  • open options page from market still doesnt work in firefox

Copy link
Member

@Step7750 Step7750 left a comment

Choose a reason for hiding this comment

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

Page layout looks good!

There's a couple of remaining pieces

  • System for fetching/storing settings (with defaults defined)
  • Defining settings for each piece, hooking it to the relevant function
  • Adding a "easy" settings button in some places of the UI (ie. in the little float bar in the Steam Market)

I think we should create a new WIP branch to merge the settings PRs into until it is ready for deployment. Otherwise, it makes it difficult to create releases on master.

src/pages/wow/index.html Outdated Show resolved Hide resolved
@ayoung19
Copy link
Contributor Author

oh i was planning on finishing up the pr on this branch and merging it when it's ready, it's still kind of a draft

@Step7750 Step7750 marked this pull request as draft July 20, 2023 06:53
@Step7750
Copy link
Member

Converted to a draft PR in the meantime, please click Ready for review when you're ready!

Putting it all in one PR is fine with me.

@ayoung19 ayoung19 marked this pull request as ready for review July 20, 2023 22:30
@ayoung19 ayoung19 requested a review from Step7750 July 22, 2023 21:32
src/background.ts Outdated Show resolved Hide resolved
src/lib/components/market/skin_viewer.ts Outdated Show resolved Hide resolved
src/lib/bridge/handlers/open_options_page.ts Outdated Show resolved Hide resolved
src/lib/bridge/handlers/open_options_page.ts Outdated Show resolved Hide resolved
src/lib/components/market/skin_viewer.ts Outdated Show resolved Hide resolved
<Flex direction="column" w={500}>
<Title order={6}>3D/Screenshot Buttons</Title>
<Text fz="xs">
Shows buttons under a listing to quickly view 3d model or screenshot of the specific item.
Copy link
Member

Choose a reason for hiding this comment

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

If we define the options that are possible in a schema (or just their own Setting<name, type, data_type>), then we can also define the description and auto-generate the entire page here.

Adding new settings would become super-simple, just adding the new defined Setting option and it all "hooks" up and allows toggling it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i agree it would be really nice once we’ve fleshed out more settings and define specific data types but it feels like an over optimization right now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

would it be fair to add this once we have a third repeating data type for our settings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually at the very least i’ll component-ize this boolean setting field rn

Copy link
Member

Choose a reason for hiding this comment

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

Sure, I think my solution is a bit of over-engineering for this. But yeah, let's prioritize making it simple to add new options fields.

@ayoung19 ayoung19 requested a review from Step7750 August 3, 2023 08:17
@ayoung19 ayoung19 force-pushed the feature/options-page branch from 822be1e to e5f6f49 Compare August 3, 2023 08:38
Copy link
Member

@Step7750 Step7750 left a comment

Choose a reason for hiding this comment

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

It seems like the PR doesn't build:

[webpack-cli] HookWebpackError: Maximum call stack size exceeded

Also looks like the build rule caught a circular dependency.

package.json Outdated Show resolved Hide resolved
@dduart3
Copy link

dduart3 commented Mar 9, 2024

May I know if this PR is going to be merged?

@dduart3
Copy link

dduart3 commented Mar 9, 2024

@Step7750

@Step7750
Copy link
Member

Step7750 commented Mar 9, 2024

May I know if this PR is going to be merged?

I believe there were some issues with getting this to run properly on Firefox at one point. If you'd like to fork it, feel free.

@ayoung19
Copy link
Contributor Author

ayoung19 commented Mar 9, 2024

Also one of the build tools that checks for import cycles wasn't happy, I can take a look there really only is like 1% left to get this merged.

@dduart3
Copy link

dduart3 commented Mar 9, 2024

Also one of the build tools that checks for import cycles wasn't happy, I can take a look there really only is like 1% left to get this merged.

I was just checking this branch, it looked good and built correctly, I was looking to create something like this but this is already too advanced to be discarded imo. I would just change some minor details and add a couple options that I have in mind already.

I'm not sure at all about the issue you're mentioning, it already passed the checks but I would love to see this being merged or approved for merge at least

@ayoung19
Copy link
Contributor Author

ayoung19 commented Mar 9, 2024

Basically the CI has a circular dependency check that isn't passing because it doesn't seem to like the import cycles that exist in React (which is normal).

But yeah how about I try fix up those two small issues preventing it to be merged and then you can build on top of that.

@dduart3
Copy link

dduart3 commented Mar 9, 2024

Basically the CI has a circular dependency check that isn't passing because it doesn't seem to like the import cycles that exist in React (which is normal).

But yeah how about I try fix up those two small issues preventing it to be merged and then you can build on top of that.

Sounds really good man, would be happy to help if you need it of course

@ayoung19
Copy link
Contributor Author

ayoung19 commented Mar 9, 2024

Yeah the bigger issue is the link to the options page (from the listing page) doesn't work on Firefox for whatever reason... A fresh pair of eyes may help with that.

@dduart3
Copy link

dduart3 commented Mar 9, 2024

Yeah the bigger issue is the link to the options page (from the listing page) doesn't work on Firefox for whatever reason... A fresh pair of eyes may help with that.

Sure thing man, I'll look into it

@ayoung19
Copy link
Contributor Author

ayoung19 commented Mar 9, 2024

@Step7750 Omg... This was the problem the whole time 😭

return !(isFirefox() && inPageContext());

@dduart3
Copy link

dduart3 commented Mar 10, 2024

@Step7750 Omg... This was the problem the whole time 😭

return !(isFirefox() && inPageContext());

is it working now? haha

@ayoung19
Copy link
Contributor Author

No, I totally missed that the content script <-> background messaging isn't supported for firefox. @Step7750 any input on how you want this to be fixed?

@Step7750
Copy link
Member

No, I totally missed that the content script <-> background messaging isn't supported for firefox. @Step7750 any input on how you want this to be fixed?

Sorry, it has been a long time (hope you're doing well @ayoung19 !)

Content Script/Background Messaging is supported on Firefox using the postMessage fallback bus:

return g_PostMessageBus.sendRequest(bundle);

@ayoung19
Copy link
Contributor Author

ayoung19 commented Dec 11, 2024

@Step7750 Thanks and hope you're doing well too! Also yes you're right I took another look and found that it's because I was calling ClientSend(OpenOptionsPage, undefined) instead of ClientSend(OpenOptionsPage, {}). It works now!

image

Also I noticed the 3D screenshot buttons are gone now so it seems like that option is deprecated. Do you have a new options you'd like added? Would love to finally close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants