-
-
Notifications
You must be signed in to change notification settings - Fork 282
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
feat(Admin System): Admin Panel Search Page and Edit Privileges #996
feat(Admin System): Admin Panel Search Page and Edit Privileges #996
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also found a couple of linting errors, try fixing those too.
About this, can you point me to a line where a linting error is arising? I have tried running linters on my local machine and all seems good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small change. Otherwise good to go
It seems that they got fixed. Also there are a few linting warnings, if you can fix them as well. |
I was running |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work so far ! You've got a pretty clean interface, working smoothly, and the code is nice and readable. Well done !
I've deployed the PR to test.bookbrainz.org for testing.
I'm very late to the party, but I have some code suggestions below. None of them are super pressing, but I'd love to see them implemented in a follow-up PR.
Looking forward to seeing the continuation !
</div> | ||
) | ||
} | ||
<h3 style={{color: '#754e37'}}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CSS classes instead of inlined styles, please :)
It's easier to reason about the CSS and keep it all synchronized when it is all in one place instead of scattered across the codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. I have created a class named search-results-heading
in this PR. Since this AdminPanelSearchResults
component is largely inspired from the SearchResults
component, I have made this change in that file as well. It was a small enough change to not warrant another PR, and so I took the liberty of including this change too.
> | ||
<thead> | ||
<tr> | ||
<th className="col-md-5">Name</th> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using bootstrap class names here does not work on Safari (we just fixed that same issue across the codebase in #987, it's fresh on my mind :) ).
You can use the width
property (native to the <th>
element) to the same effect. The value should be width="42%"
which is what col-md-5 does, although I have found that the following values work well:
size: PropTypes.number, | ||
updateResultsTrigger: PropTypes.number.isRequired |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above.
} | ||
} | ||
|
||
/* eslint-disable no-bitwise */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You know, there's rarely a good use for disabling linting rules, but if I ever need a good example this will be the perfect one ! :D
<Modal.Header closeButton> | ||
<Modal.Title> | ||
<img height="20" src={getPrivilegeShieldIcon(this.state.privs)}/> {' '} | ||
<a href={link}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this should be a link to the user's profile. It might be clearer to have this be just text, and have a separate "visit profile" link in the modal body (perhaps opening in a new tab).
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, I have removed the link to the user's profile in this follow up PR. I will, however, be sure to take care of this "Visit Profile" functionality in the next iteration of the PrivsModal.
const privilegesDropdownTitle = ( | ||
<span> | ||
<FontAwesomeIcon icon={faShieldHalved}/> | ||
{' Privileges'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't need to be quoted and in a JSX block, it can just be the text content
> | ||
<NavDropdown.Item href="/admin-panel"> | ||
<FontAwesomeIcon icon={faUserGear}/> | ||
{' Admin Panel'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, this can be just the text
<img height="18" src={getPrivilegeShieldIcon(result.privs)}/> {' '} | ||
<FontAwesomeIcon icon={faPencilAlt}/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder about this; it seems to me that the shield icon only repeats the information we already have in the privileges column.
I think we only need an "Edit" button for now (in the future maybe "Block"/"Delete" buttons as well), for which I'd rather have a text button that the very small icon which is harder to click on.
What do you think?
Here it is with text, and some button classes (btn-sm for size and btn-success for style)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shield icons were never really for any kind of additional information. They were just an aesthetic choice, one which I dont mind rescinding. One other reason I clubbed them with the pencil icon was so I can increase the hitbox of the click.
I did give some thoughts to how the edit button should look like and here's why I ditched this simple Edit button. The main reason is because I think these buttons, when iterated in a table look really out of place to me, mostly because these types of buttons have mostly been used throughout the site as singular action initiators(i made that up), basically you find them mostly in isolation (as a 'Submit' button in a page, or a 'Create a collection' button, etc). I find that they look a bit odd when iterated in a table:
This also reminds me that I've been in a similar dilemma before, in this old PR where I used a question mark as a clickable link. I do agree that it does not work very good as a call to action and I also agree that we can definitely look for a better alternative in that case too, but now you know why I went for it instead of a button.
I do realise that this can all be a very subjective discussion, and it is quite possible that I may be reading too much into this. I am okay with changing it to an 'Edit' button as suggested for now, but I think we should later go through some more ideas on what kind of buttons we'll use in a table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shield icons were never really for any kind of additional information.
I think they look neat, but in this case they should be used in another place if they repeat info from the previous column. Maybe they would be a good indicator in front of user names on the website? Possibly only showing admins and privileged users, and not showing the empty shield icon in front of every user's name.
For this table, if you were to add it, I would add it in front of the user name instead of the user icon (which currently doesn't add any info). Might be helpful when quickly reading through the table.
I get your thought process about the action column; the repeated buttons are perhaps not ideal, but IMO they are more usable that just the icon (as you mention the hitbox is small) even if the icons look better. The issue will compound once we have more actions.
I think it'll end up looking fine with multiple buttons, each having a different color will help prevent misclicks (when compared to standalone icons).
If you want to jazz them up, I have two suggestion:
- we have outline style buttons which could reduce the repeating color block issue (i.e.
btn-outline-success
class etc. for -info, -danger, -warning etc.) - you can add the icon in the button too
Overview:
This PR adds the functionality which allows an Administrator to search for users and then change their privileges.
Components Added:
Privileges Dropdown: A privileges dropdown has been added in the NavBar, which will eventually contain options corresponding to the special privileges that a user has. Currently only the Admin Panel can be accessed using this dropdown. Ideally, this Admin Panel can be accessed only by users who have the Administrator priv. We'll later use middlewares to fix these.
Admin Panel Search Page: As the name implies, it allows us to search for users and makes use of two sub-components named:
AdminPanelSearchField
andAdminPanelSearchResults
. The functionality of this is largely inspired from the Search Page, which we already have.Privs Edit Modal: A modal which allows us to change privileges.
Privilege Badges: This component displays the titles of the privileges in the form of badges.
Icons:
I have taken some liberty and added some icons to differentiate users depending on the sets of privileges they have. They are used in the
PrivEditModal
andAdminPanelSearchResults
components and brief outline of what they represent is as follows: