aprzn123 / TheThirdCan

A browser extension designed to improve https://twocansandstring.com
MIT License
7 stars 4 forks source link

Improve configuration for boolean config options #39

Closed aprzn123 closed 1 year ago

aprzn123 commented 1 year ago

I wasn't able to do as much as I had hoped (might not be worth merging, but I think it is anyway), but the main benefit is that if you are adding a boolean config value, you only need to modify config.js in one place instead of several places like before. Also, fixed a little bug with skip for now not showing up. Other than this, I think the code rework is nearing completion! We can discuss more in the main issue, but LMK if there are other things you think we need to address first.

HumanoidSandvichDispenser commented 1 year ago

I think the same should also be done for radio buttons and all other config types. What's stopping us from doing that, or this might seem overkill, but what about some kind of webapp framework for the popup?

aprzn123 commented 1 year ago

I was considering trying out Preact for the popup, but I did think that sounded like overkill.

The main issue with handling the radio buttons with the same system is that they're so much more varied, with one name for the variable, and then a separate id for each radio button. If you have any suggestions for how to put together a good API for that, I could try to implement it.

HumanoidSandvichDispenser commented 1 year ago

You could change the value attribute of these radio buttons to 0, 1, 2 instead of "neverSkipConfirm", etc.

https://github.com/aprzn123/TheThirdCan/blob/272d3a74a2f1d5da59d9f9abb722c48842d51d0d/src/popup/config.html#L13-L20

and then to update the radio buttons, you could use this query selector and set the element's checked property to true

input[name="radio button name"][value="config value"]

and then to update settings, you could use the query selector and get the value property

input[name="radio button name"]:checked

That way, you don't have to manually hardcode loading and saving radio button values

aprzn123 commented 1 year ago

Speaking of that, we should probably put some documentation somewhere about the meaning of the numbers, so that we can just use the numbers everywhere. Documentation in general will probably be a higher priority moving forward.

HumanoidSandvichDispenser commented 1 year ago

What's the current status of this?

aprzn123 commented 1 year ago

I was trying to incorporate an api for enum values, but I'm stuck with a very strange error where I'm calling document.getElementById() and it's returning null, despite there definitely being an element with the given id. I'll push what I have to here for now, can you take a look?

aprzn123 commented 1 year ago

The config system has been extended to enums now. It's a shame that there's no actual enums in this language; we'll just have to stick with well-documented integers ig

roxwize commented 1 year ago

We have the global variable stuff. Can't we just put enumerators in there? Or am I misguided? I feel like I'm misguided. I haven't interacted with this in a while and I'm still confused.

aprzn123 commented 1 year ago

You're right, that just didn't occur to me for some reason. I'm really just not used to doing things like this without a strong type system lol

aprzn123 commented 1 year ago

Good news: I may be out of procrastination mode again! Might just take executive action and merge this into rework immediately so that hopefully we can just get a good-enough rework out of the way so we can get back to real features. Once we get this shit over and done with and add a couple new features, I'll make an announcement on the forums that we're back in action (for now at least lol)

aprzn123 commented 1 year ago

ok now it's good enough i think, merging