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

Derive initial checkbox state from query string #220

Merged
merged 3 commits into from
Dec 30, 2024
Merged

Conversation

cead22
Copy link
Contributor

@cead22 cead22 commented Dec 24, 2024

Tested various initial states

one two
image image
image image

And tested toggling the checkboxes

image

@cead22 cead22 requested a review from tgolen December 24, 2024 18:53
@cead22 cead22 self-assigned this Dec 24, 2024
Copy link
Contributor

@tgolen tgolen left a comment

Choose a reason for hiding this comment

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

I guess this is fine, but I feel like if I allow it, I'm just gonna write another PR that does this using Onyx properly. The query params are not ideal because they are not discoverable. The only way people will use them is if they are specifically told and educated about them, and I think that's doing a disservice to the feature.

Also, in order to get a PR merged and deployed, you'll need to follow all the instructions in the readme. There are several version files you need to bump, for example.

@cead22
Copy link
Contributor Author

cead22 commented Dec 30, 2024

I guess this is fine, but I feel like if I allow it, I'm just gonna write another PR that does this using Onyx properly.

Is that NAB?

The query params are not ideal because they are not discoverable. The only way people will use them is if they are specifically told and educated about them, and I think that's doing a disservice to the feature.

I personally think that's fine for now. I don't know that anyone but me wants this right now

@tgolen
Copy link
Contributor

tgolen commented Dec 30, 2024

OK, if that's what you would like, then sure :)

@cead22
Copy link
Contributor Author

cead22 commented Dec 30, 2024

Haha yeah, I'd prefer shipping this quickly even if we decide to refactor it later, over having to learn how to re-implement this

@cead22
Copy link
Contributor Author

cead22 commented Dec 30, 2024

Updated

@tgolen tgolen merged commit 1fb35dc into main Dec 30, 2024
3 checks passed
@tgolen tgolen deleted the carlos-querystring branch December 30, 2024 20:19
@tgolen
Copy link
Contributor

tgolen commented Dec 30, 2024

I'll leave it to you to deploy this!

@cead22
Copy link
Contributor Author

cead22 commented Dec 30, 2024

image

I submitted it for review on the chrome web store. Thanks for your help here. Do you think I should send an email to all@ to let everyone know about this feature?

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.

2 participants