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

Various mod improvements #1864

Open
wants to merge 10 commits into
base: eth-master
Choose a base branch
from
Open

Various mod improvements #1864

wants to merge 10 commits into from

Conversation

jjeffryes
Copy link
Contributor

This adds the same changes from #1859 and #1861 to the eth-master branch.

This also closes #1862.

This is built on top of #1863, and should not be merged until that branch is merged. The WIP label is just there to make sure that doesn't happen.

This was referenced Jan 17, 2020
@rmisio
Copy link
Contributor

rmisio commented Jan 30, 2020

@jjeffryes Would you mind updating this with the latest eth-master (I merged #1863) so the change-set is a little lighter?

@jjeffryes
Copy link
Contributor Author

@rmisio I merged in eth-master, it's a lot more sane now. :)

@rmisio
Copy link
Contributor

rmisio commented Feb 3, 2020

When I search for a mod, I get double "loading" texts:

image

@@ -125,8 +132,10 @@ export default class extends BaseVw {

loadTemplate('components/moderators/card.html', (t) => {
this.$el.html(t({
valid: !!this.model.isValid(),
Copy link
Contributor

Choose a reason for hiding this comment

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

isValid returns a bool so the negated not is not needed.

Also, should we maybe name the variable validModel? valid seems so ambiguous.

@@ -3,7 +3,8 @@
const loaded = !!ob.name;
/* Disable the card if it is invalid and the controls should be shown, and it is not selected. This allow the user to de-select invalid cards.
The view should prevent the invalid card from being selected again, disabling it is redundant but important visually. */
const isDisabled = (!ob.valid && !ob.controlsOnInvalid ) || (!ob.valid && ob.controlsOnInvalid && ob.selectedState !== 'selected') || !loaded ? 'disabled' : '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we clean up the indentation on the comment? It's kind of all over the place.

A common pattern we use is:

/*
 * Hey, I'm a multi-line block comment.
 * Don't I look purty?
 */

@@ -3,7 +3,8 @@
const loaded = !!ob.name;
/* Disable the card if it is invalid and the controls should be shown, and it is not selected. This allow the user to de-select invalid cards.
The view should prevent the invalid card from being selected again, disabling it is redundant but important visually. */
const isDisabled = (!ob.valid && !ob.controlsOnInvalid ) || (!ob.valid && ob.controlsOnInvalid && ob.selectedState !== 'selected') || !loaded ? 'disabled' : '';
const isOKMod = ob.valid && ob.isMod;
const isDisabled = (!isOKMod && !ob.controlsOnInvalid ) || (!isOKMod && ob.controlsOnInvalid && ob.selectedState !== 'selected') || !loaded ? 'disabled' : '';
Copy link
Contributor

Choose a reason for hiding this comment

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

I would maybe rename this to disabledClass to more accurately represent what the variable holds. isDisabled leads me to believe its a boolean.

this.processMod(eventData);
}
} else if (eventData.id === asyncID && !excluded.includes(eventData.peerId)) {
} else if (
eventData.id &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these first two really needed. If either was not truthy, wouldn't one or both of the last two conditions fail?

@@ -86,7 +91,9 @@ export default class extends BaseVw {
}

get hasValidCurrency() {
return anySupportedByWallet(this.modCurs);
const validFeeCur = this.fixedFeeCur &&
Copy link
Contributor

Choose a reason for hiding this comment

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

I would just:

this.fixedFeeCur && !!getCurrencyByCode(this.fixedFeeCur)

We should avoid sorting the list since that's not needed here.

// provide the expected capitalization of peerID
eventData.peerID = eventData.peerId;
delete eventData.peerId;
if (this.modsToFetch.includes(eventData.peerID)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

the "peer id" coming from the socket is peerId not peerID.

@rmisio
Copy link
Contributor

rmisio commented Feb 4, 2020

@jjeffryes One other thing I noticed is that it appears that when I toggle the Verified Moderators checkbox the whole payment screen is re-rendering. I haven't looked in the code, but I'm basing that on the fact that the images across the whole view flicker.

If that's the case, seems way too heavy handed. I would think that upon toggle just the moderators view would need some state updated.

Anyhow, I know that probably wasn't introduced in this PR, so let me know if it's better I just log an issue we could prioritize for some other time?

@rmisio
Copy link
Contributor

rmisio commented Feb 4, 2020

@jjeffryes Also please let me know if I should log a seperate issue for this or if you want to fix it in this PR:

#1862 (comment)

@jjeffryes
Copy link
Contributor Author

@jjeffryes One other thing I noticed is that it appears that when I toggle the Verified Moderators checkbox the whole payment screen is re-rendering. I haven't looked in the code, but I'm basing that on the fact that the images across the whole view flicker.

If that's the case, seems way too heavy handed. I would think that upon toggle just the moderators view would need some state updated.

Anyhow, I know that probably wasn't introduced in this PR, so let me know if it's better I just log an issue we could prioritize for some other time?

I'll see if I can fix that with a reasonable amount of effort when revising this PR, if not I'll create a new issue for it.

@jjeffryes jjeffryes changed the title [WIP] Various mod improvements Various mod improvements Feb 6, 2020
@jjeffryes
Copy link
Contributor Author

I made a separate issue for future rendering optimization. #1866

The rest of this should be ready for re-review.

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.

2 participants