Skip to content
This repository has been archived by the owner on Mar 28, 2023. It is now read-only.

Refactor mod loading #1859

Closed
wants to merge 10 commits into from
Closed

Refactor mod loading #1859

wants to merge 10 commits into from

Conversation

jjeffryes
Copy link
Contributor

This cleans up and improves the handling of loading mods.

It also fixes an issue where sometimes the empty placeholder was being shown instead of one of the messages that explained no moderators were being shown. Now the loading message will be shown while loading, and one of the other messages will be shown if no valid moderators were loaded.

Closes #1851

@jjeffryes jjeffryes requested a review from rmisio January 14, 2020 17:00
@@ -162,7 +162,7 @@ export default class extends BaseModal {
singleSelect: true,
radioStyle: true,
initialState: {
showOnlyCur: currencies[0],
showOnlyCur: '',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was only showing moderators that accept BTC, I assume it was there because originally we automatically selected the first currency. Since we changed it to require the user to select a currency (none are selected by default) this should also have no default.

@@ -38,7 +38,7 @@ export default class extends BaseVw {
let mode = this.getState().mode;
if (mode === 'loadingXofY') mode = 'loadingXofYTimedOut';
this.setState({ showSpinner: false, mode });
}, 10000);
}, 60000);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made this longer since I was seeing the message for no mods appear then mods would load, which was pretty clunky.


// Either a list of IDs can be posted, or any available moderators can be retrieved with GET
if (IDs.length || op.method === 'GET') {
if (this.modsToFetch.length || op.method === 'GET') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was moved so it wouldn't set the state to loading if there were no mods to fetch.

@jjeffryes
Copy link
Contributor Author

Closed while I fix a bug.

@jjeffryes jjeffryes closed this Jan 14, 2020
@jjeffryes jjeffryes reopened this Jan 14, 2020
</div>
</div>
<% } %>
<% if (ob.totalIDs && !ob.totalShown) { %>
Copy link
Contributor Author

@jjeffryes jjeffryes Jan 14, 2020

Choose a reason for hiding this comment

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

The placeholder got refactored so the messages below take care of everything with less duplication. This also makes sure there's always a message.

@@ -93,9 +94,9 @@ export default class extends baseVw {

super(opts);
this.options = opts;
this.excludeIDs = opts.excludeIDs;
this.excludeIDs = [...opts.excludeIDs, app.profile.id];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seemed a lot smarter just include the user's own profile here, instead of adding it later.

We could add a parameter to not exclude it in the future if that use case ever came up.

@@ -286,22 +286,11 @@ export default class extends baseVw {
const remAvail = this.modsAvailable.removeModeratorsByID(this.modsAvailable.selectedIDs);

this.modsByID.excludeIDs = this.currentMods;
this.modsByID.moderatorsStatus.setState({
hidden: true,
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These three sections of code were removed, they were just duplicating things already handled by the view and creating some weird edge case behavior (like if you loaded moderators, then closed and reopened settings the status bar would appear).

mod.model.hasModCurrency(state.showOnlyCur) && !mod.model.isVerified).length;
const totalIDs = this.allIDs.length;
const unVerCount = this.modCards.filter(mod => (!state.showOnlyCur ||
mod.model.hasModCurrency(state.showOnlyCur)) && !mod.model.isVerified).length;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since a default currency isn't being passed in (removed below), this was updated to be count all the unverified moderators if there is no currency set.

@rmisio
Copy link
Contributor

rmisio commented Jan 15, 2020

This one happens on this branch:

#1862

Not sure if it was introduced by this branch since I haven't tested it on master. If it does happen on master we could punt on it and prioritize later.

@jjeffryes
Copy link
Contributor Author

This is closed in favor of #1864, since non-critical updates should be happening on eth-master instead.

@jjeffryes jjeffryes closed this Jan 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Messaging needed when no valid mods are available
2 participants