-
Notifications
You must be signed in to change notification settings - Fork 6
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
Extension preferences #1341
Extension preferences #1341
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.
Nice, thanks @gsteinLTU! I have a couple suggestions around naming and the API for extensions:
- So far I have been trying to use classes for the different extension concepts to prevent silent errors due to typos and such (example). This is a bit of a deviation from the style used in other places in Snap (like here). It might be nice to structure this code similarly where there is a class like
Option
or something which then can configure itself via a builder-esque pattern. - I would probably name it
Settings
rather thanPreferences
but either is really fine.
(test? on : off), | ||
pref.label | ||
], | ||
pref.toggle, |
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 need a pref.toggle.bind(pref)
though I imagine it will probably be fine in most cases.
On the topic of documentation, I would add a test here: https://github.com/NetsBlox/Snap--Build-Your-Own-Blocks/blob/master/test/extensions.spec.js We need to add documentation about extensions. It would probably be nice to add it in the "Development" section of the main docs hosted at https://editor.netsblox.org/docs/. It looks I deleted them during the migration to the new cloud so I will need to add them back... |
3afa6b6
to
bcf5af0
Compare
@brollb Let me know if the ExtensionSetting class is what you meant. I renamed things to "Settings" in the code, although I think "Options" sounds better in the UI, since they're all boolean (for now?). |
LGTM. Thanks, @gsteinLTU! |
Close #1340
This allows extensions to optionally provide a list of toggle-able settings to be added to the menu.
@brollb Should this be called something different from "Options" in the menu, e.g. "Settings", "Preferences"? Where should the structure be documented?