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: extensions review feedback #283

Merged
merged 1 commit into from
Sep 19, 2023
Merged

Fix: extensions review feedback #283

merged 1 commit into from
Sep 19, 2023

Conversation

jmmaranan
Copy link
Collaborator

forge-ext's extension, "Forge", version 74 has a new review:

JustPerfection posted a review on September 18, 2023:

  1. Please remove the version from imports,
    because parent is the owner of prefs window and already specified the version:

    • line 20-21 prefs.js
    • line 4 lib/prefs/keyboard.js
    • line 4 lib/prefs/appearance.js
    • line 5-6 lib/prefs/widgets.js
    • line 3 lib/prefs/settings.js
  2. Typo :p in

    • line 30 prefs.js (class name)
    • line 199 lib/extension/utils.js orientaton
  3. You need to create objects on enable and null them out in disable (line 34 and 36 extension.js).
    Garbage collector cannot do it's job otherwise.

  4. Selective disable is not allowed (line 69 extension.js):
    https://gjs.guide/extensions/review-guidelines/review-guidelines.html#session-modes

Please use the review page to follow up:

https://extensions.gnome.org/review/45065

@jmmaranan jmmaranan merged commit 11a6f36 into main Sep 19, 2023
2 checks passed
@jmmaranan jmmaranan deleted the fix-ego-review-74 branch September 19, 2023 20:03
@bennypowers
Copy link
Contributor

Nice

The versioned imports were to appease the @girs/* typings. They should be made aware of it if they don't already know

@jrahmatzadeh
Copy link

@bennypowers If you mean for Gtk and Gdk, they are already specified here.

@bennypowers
Copy link
Contributor

What I mean is that in order to pick up the types from the gtk and GDK packages then the query parameters must be included I believe this could be fixed with a single typing package for extensions which also includes the resources

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