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

Addon store #1193

Open
wants to merge 96 commits into
base: main
Choose a base branch
from
Open

Addon store #1193

wants to merge 96 commits into from

Conversation

Tropix126
Copy link
Member

@Tropix126 Tropix126 commented Jan 30, 2022

@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)
  • Debug some public servers issues.
  • Some strings are hardcoded in english and need to be internationalized.
  • Ability to disable the store.
  • Store Protocol removed due to to discord making this sort of patching impractical :(
  • Uninstall button on installed addon cards.
  • Pressing Enter or Space should execute the install script in the installation modal
  • Pressing Shift and clicking an install/delete button should skip the modal and perform the action.
  • Setting (or checkbox in the install modal) to enable an addon directly after install.
  • Clicking a betterdiscord:// protocol link should redirect to the addon's page on the site.

@dav1312
Copy link
Contributor

dav1312 commented Jan 30, 2022

Editing plugins/themes on a detached window opens the file in the system editor

@dav1312
Copy link
Contributor

dav1312 commented Jan 31, 2022

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.

@Tropix126
Copy link
Member Author

Tropix126 commented Jan 31, 2022

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.

@TheGreenPig
Copy link
Contributor

TheGreenPig commented Jan 31, 2022

Once finished, store pages will likely resemble the pages 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.

@Tropix126
Copy link
Member Author

Tropix126 commented Jan 31, 2022

Once finished, store pages will likely resemble the pages 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.

@dav1312
Copy link
Contributor

dav1312 commented Feb 3, 2022

Not directly related to the store but a small idea for the changelog banner for when it's added?
Using https://nyri4.github.io/Discolored/assets/empty.svg
store

@TheGreenPig
Copy link
Contributor

TheGreenPig commented Feb 23, 2022

Some things I noticed:

  • Clicking the card throws a this.props.onDetailsView is not a function error. (Probably just not implemented yet, I thought I'd mention it here anyway, I think the plan is to show the README there?)
  • Maybe a "Do not show this again" checkbox could be added to the popout that get's shown after you press the install button? The popup is nice, but it might get annoying every time you want to install an addon... the other option I think would be good is not display the info when clicking the install button, but adding an "info" button that will display the popout instead.
  • Just an idea, but maybe the install button could turn into a red uninstall button if the addon is installed already, instead of making the button disabled? If you want to test out a theme for example and you try it, but decide you don't like it right away, it's easier to have a button there to uninstall it instead of having to go to the other tab, find the theme and delete it there...
  • How about being able to filter by multiple tags?
  • Not sure if I'm missing the feature, but maybe the support server invite could be added in the info popoup (or as it's own icon) as well? It looks like guild data is already passed in the addon props I think, so maybe you're already planning to do so...
  • I think you can sort by "Date added", but not actually view when it was added anywhere? The info popoup shouldn't be bloated with information for sure, but still, I feel like if you can sort by "Date added" you should be able to view that data as well...

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...

@Strencher
Copy link
Member

Clicking the card throws a this.props.onDetailsView is not a function error [...[

Yes, this is because the store was disabled temporarily due to crashes.

Maybe a "Do not show this again" [...]

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.

  • Just an idea, but maybe the install button could turn into a red uninstall button if the addon is installed already [...]
  • How about being able to filter by multiple tags?
  • I think you can sort by "Date added", but not actually view when it was added anywhere? [...]

Yes. I can out in on our todo list.

@TheGreenPig
Copy link
Contributor

A setting to automatically enable the addon after it was downloaded would also be nice...

@Tropix126
Copy link
Member Author

It doesn't work after 95ccea8

Fixed, turns out the ProvidePlugin was "providing" the react import to a single file that didn't import react lol.

- 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
Copy link
Contributor

@dav1312 dav1312 left a 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 | 

renderer/src/styles/ui/store.css Outdated Show resolved Hide resolved
renderer/src/styles/ui/store.css Outdated Show resolved Hide resolved
renderer/src/styles/ui/store.css Outdated Show resolved Hide resolved
renderer/src/ui/modals.js Outdated Show resolved Hide resolved
Copy link
Member

@rauenzi rauenzi left a 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

renderer/src/modules/addonmanager.js Outdated Show resolved Hide resolved
renderer/src/modules/addonmanager.js Show resolved Hide resolved
renderer/src/styles/ui/markdown.css Outdated Show resolved Hide resolved
renderer/src/ui/addonerrormodal.jsx Show resolved Hide resolved
renderer/src/ui/settings.js Outdated Show resolved Hide resolved
renderer/src/modules/bdwebapi.js Outdated Show resolved Hide resolved
Copy link
Member

@Strencher Strencher left a 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.

renderer/src/builtins/addons/store.js Outdated Show resolved Hide resolved
renderer/src/ui/settings/addonlist/store.jsx Show resolved Hide resolved
renderer/src/ui/settings/addonlist/installed.jsx Outdated Show resolved Hide resolved
Copy link
Member

@rauenzi rauenzi left a 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

@rauenzi
Copy link
Member

rauenzi commented Oct 14, 2022

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";
Copy link
Member

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.

Suggested change
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]);
Copy link
Member

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.

Suggested change
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);
Copy link
Member

Choose a reason for hiding this comment

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

Same goes here.

Suggested change
reject(error);
return reject(error);

request(Web.ENDPOINTS.addon(addon), (error, _, body) => {
if (error) {
Logger.stacktrace("WebAPI", Strings.Store.connectionError, error);
reject(error);
Copy link
Member

Choose a reason for hiding this comment

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

Same goes here.

Suggested change
reject(error);
return reject(error);

request(Web.ENDPOINTS.download(id), (error, _, body) => {
if (error) {
Logger.stacktrace("WebAPI", Strings.Store.connectionError, error);
reject(error);
Copy link
Member

Choose a reason for hiding this comment

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

Same goes here.

Suggested change
reject(error);
return reject(error);

@rauenzi
Copy link
Member

rauenzi commented Feb 26, 2024

@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 v1.10.0 releases I might just make a branch and squash merge this into it and see if I can mash things together in a somewhat clean way since the codebase has made so many changes since this.

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.

None yet

7 participants