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

Allow site admins to manage other site admins #1404

Merged
merged 3 commits into from
Jan 7, 2016
Merged

Conversation

jkeck
Copy link
Contributor

@jkeck jkeck commented Jan 5, 2016

Closes #849

admin-user-admin

@@ -75,5 +75,5 @@


Spotlight.onLoad(function() {
$('.edit_exhibit').spotlight_users();
$('.edit_exhibit, .admin-users').spotlight_users();

Choose a reason for hiding this comment

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

Identifier 'spotlight_users' is not in camel case.

@jkeck
Copy link
Contributor Author

jkeck commented Jan 5, 2016

There is a bit of a spacing issue in the form where the label has .sr-only as well as a .col-md-2 but the .col-* declaration is keeping the allotted space available. I don't see why the .col-* class isn't being added other places where we're using hide_label: true.

If anybody has any ideas as to why this is happening or proposed solutions, I'm all ears.

@jkeck
Copy link
Contributor Author

jkeck commented Jan 5, 2016

Odd about the reduced coverage. I think it's caused by relevant lines of code being moved, but that code is tested elsewhere (and showing 100% coverage). It seems strange to me that this would cause a drop of 0.2%.

@cbeer
Copy link
Member

cbeer commented Jan 5, 2016

Agreed about the coverage; the coveralls report is less than helpful.

Should we wait to merge this until we can look at the label spacing problem?

@jkeck
Copy link
Contributor Author

jkeck commented Jan 5, 2016

Yeah, maybe I can show you what is going on. It's pretty minor so we can fix it up and :shipit: later today or tomorrow.

@cbeer
Copy link
Member

cbeer commented Jan 5, 2016

👌

@cbeer cbeer changed the title Allow site admins to manage other site adminis Allow site admins to manage other site admins Jan 6, 2016
@cbeer
Copy link
Member

cbeer commented Jan 7, 2016

I've fixed the label padding problem -- it looks like the horizontal form layout (which we didn't actually seem to need) forced columns on us.

screen shot 2016-01-07 at 8 09 24 am

@ggeisler
Copy link
Contributor

ggeisler commented Jan 7, 2016

👍

@jkeck
Copy link
Contributor Author

jkeck commented Jan 7, 2016

Ah! Makes perfect sense. Thanks @cbeer!

@cbeer
Copy link
Member

cbeer commented Jan 7, 2016

I'm not sure about perfect sense, but it probably helps keeping things aligned (and if we weren't using tables, we might need it..)

cbeer added a commit that referenced this pull request Jan 7, 2016
Allow site admins to manage other site admins
@cbeer cbeer merged commit 86e5162 into master Jan 7, 2016
@cbeer cbeer deleted the 849-super-admins branch January 7, 2016 16:49
@cbeer cbeer removed the in progress label Jan 7, 2016
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

4 participants