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
Addon store #1193
base: main
Are you sure you want to change the base?
Addon store #1193
Conversation
Editing plugins/themes on a detached window opens the file in the system editor |
When you click on an addon to see the readme, there isn't really a way to go back. And if you press "installed" and then "store" to go back to the store, all tags are gone. |
Aware of this. Addon detail (store pages) are unfinished and will be rewritten in the near future. It's likely that they will be moved to their own LayerContainer, since one thing I've proposed in the past is opening a store page by ID using a protocol (something like betterdiscord://store/8) and that will be easier if pages are self-contained. It'll also give us more breathing room in the layout. Once finished, addon detail pages will likely resemble the ones seen here. |
Just visited this site and it looks good, but maybe 0.1m could be changed to 100k? It's rather inaccurate and not really pretty to have values below 1 in my opinion. |
This page is rather old. I worked on it shortly after the initial release of the site around a year ago. It's very rough around the edges and is not used anywhere in this PR. AFAIK we use a different method of abbreviating stats in the actual store. All I plan to borrow is the rough idea of its design on the addon detail pages. |
Not directly related to the store but a small idea for the changelog banner for when it's added? |
Some things I noticed:
So I obviously don't know what your future plans are/if you have considered or will implement the things I listed above/even want feedback at all, but maybe this is still useful feedback... |
Yes, this is because the store was disabled temporarily due to crashes.
That's a good idea. But although I'd rather make it a feature such as holding shift to install immediately. I'll add it to the todo.
Yes. I can out in on our todo list. |
A setting to automatically enable the addon after it was downloaded would also be nice... |
Fixed, turns out the |
- Moves all `AddonManager` and `Toasts` usage out of `BdWebApi` to prevent circular dependencies. -Replaces `BdWebApi.installAddon` with `getAddonContents` and an `AddonManager.installAddon` - Move store-specific strings into `Strings.Store` (rather than `Strings.Addons`). - TabBar component that's actually accessible. Replaces the previous inaccessible one in the store and `AddonErrorModal`. - StoreCard is now Pure. - InstallationModal is now consistent with StoreCard's props terminolgy. - search.js => searchbar.js for consistent naming. - Better error handling for everything. - lint
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.
ERROR in ./src/styles/index.css
Module build failed (from ./node_modules/postcss-loader/dist/cjs.js):
SyntaxError
(431:5) postcss-csso: /home/runner/work/BetterDiscord-App/BetterDiscord-App/renderer/src/styles/ui/store.css Identifier is expected
429 | .theme-light .bd-store-tags span.selected {
430 | background-color: var(--brand-experiment, hsl(235, calc(var(--saturation-factor, 1) * 85.6%), 64.7%));;
> 431 | color: #fff;
| ^
432 | }
433 |
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.
Couple minor things/changes but otherwise LGTM
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.
Looks good to me otherwise.
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.
The only thing I would change is having done a git mv
of renderer/src/ui/settings/addonlist.jsx
to renderer/src/ui/settings/addonlist/installed.jsx
so the history and diff are maintained. Otherwise LGTM
I will wait for Strencher to give a look over the latest review and we will probably move the target to the development branch |
import Utilities from "./utilities"; | ||
import Strings from "./strings"; | ||
|
||
import request from "request"; |
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.
Since we emulate the request
module, it'd be better off importing from ../polyfill/request
instead of adding a race condition.
import request from "request"; | |
import request from "../polyfill/request"; |
*/ | ||
getAddons(type) { | ||
return new Promise((resolve, reject) => { | ||
if (API_CACHE[type].length) resolve(API_CACHE[type]); |
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.
This needs a return
statement, otherwise the code will still run below.
if (API_CACHE[type].length) resolve(API_CACHE[type]); | |
if (API_CACHE[type].length) return resolve(API_CACHE[type]); |
request(Web.ENDPOINTS.store(type), (error, _, body) => { | ||
if (error) { | ||
Logger.stacktrace("WebAPI", Strings.Store.connectionError, error); | ||
reject(error); |
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.
Same goes here.
reject(error); | |
return reject(error); |
request(Web.ENDPOINTS.addon(addon), (error, _, body) => { | ||
if (error) { | ||
Logger.stacktrace("WebAPI", Strings.Store.connectionError, error); | ||
reject(error); |
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.
Same goes here.
reject(error); | |
return reject(error); |
request(Web.ENDPOINTS.download(id), (error, _, body) => { | ||
if (error) { | ||
Logger.stacktrace("WebAPI", Strings.Store.connectionError, error); | ||
reject(error); |
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.
Same goes here.
reject(error); | |
return reject(error); |
@dav1312 who owns the rights to that image you posted #1193 (comment) it looks great and would be cool to use for changelogs as you suggest. Overall I'd like to salvage this code as much as I can. Once |
@Strencher decided to fix some of the work he and I did earlier last year. This is still somewhat work-in-progress, but is fairly stable and fully functional.
todo:
Finish (or temporarily disable) store pages.(temporarily disabled until README-related stuff is figured out further on the site backend)Store Protocolremoved due to to discord making this sort of patching impractical :(betterdiscord://
protocol link should redirect to the addon's page on the site.