-
Notifications
You must be signed in to change notification settings - Fork 174
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
[issue_tracker] Convert batch/normal mode toggle to tabs and implement permission control #9434
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.
lgtm but did not pull/test. thanks for taking care of this so quickly Ayush
@ay-bh confirm pls this doesn't create bottlenecks by loading all the data in both tabs at once, correct?
as a future enhancement we could consider hiding the Browse Issues
tab for non-developers - the vertical space use doesn't make sense if it's the only tab option for end-users. @regisoc @racostas any strong feelings here?
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.
Thanks @ay-bh !
- PR rebase required.
- This looks clean. I like it.
- @christinerogers Agreed, tabs should be hidden for someone not having the right permission i.e. this could be completely hidden:
9c7b0fc
to
a751f83
Compare
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.
LGTM.
- Tested with various uses/permissions looks good to me.
- The tabs yet appear when the user have only the permission "not developer" but I think this is more cosmetic and should not stop us form merging this one
What do you think @regisoc , @christinerogers ?
@racostas that is a cosmetic change, we can change it later. |
addressed. @regisoc confirmed in the thread
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.
@christinerogers just to confirm.
3 approvals - over to @driusan for final review. |
Brief summary of changes
Tabs
componentissue_tracker_developer
permissionTesting instructions
issue_tracker_developer
permission - verify only "Browse Issues" tab is visibleissue_tracker_developer
permission - verify both "Browse Issues" and "Batch Edit" tabs are visibleLink(s) to related issue(s)
developer
permission #9422, [issue_tracker] Batch mode: Button style should match LORIS #9423Screenshot