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

Several improvements to adminscore #134

Merged
merged 2 commits into from Nov 28, 2017
Merged

Several improvements to adminscore #134

merged 2 commits into from Nov 28, 2017

Conversation

Matthewrbowker
Copy link
Member

T179508
T179764
T179763

T179508
T179764
T179763
Copy link
Member

@MusikAnimal MusikAnimal left a comment

Choose a reason for hiding this comment

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

Good stuff! Just a few comments, and looks like there are some codesniffer bugs.

<span class="text-muted small">
{{ msg(key~"-desc", [master[key].value, master[key].value|number_format]) }}
</span>
</td>
Copy link
Member

Choose a reason for hiding this comment

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

With the extra column, the text is getting wrapped. Changing stat-list div to have col-lg-12 instead of 6 should do it

This also doesn't show up very well on mobile, but not a big deal. I can't think of a straightforward solution off of the top of my head.

@@ -48,6 +51,7 @@
"block-log": "Block log",
"block-longest": "Longest block",
"blocks": "Blocks",
"blocks-desc": "$2 {{PLURAL:$1|block|blocks}} administered",
Copy link
Member

Choose a reason for hiding this comment

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

Just curious, why do we include this in the score? I thought Admin Score was more for non-admins. Even for admins, it seems odd to only account for blocks and not other admin actions.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not quite sure, our AdminScore was copied from the old one entirely. I would be OK removing it, maybe we should open a Phabricator task to discuss it?

Copy link
Member

Choose a reason for hiding this comment

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

Nah I think it's reasonable to just remove it. Doesn't have to be part of this PR, though!

@Matthewrbowker Matthewrbowker merged commit e6cb365 into master Nov 28, 2017
@Matthewrbowker Matthewrbowker deleted the admin-score branch November 28, 2017 00:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants