|
Description
Fridolin Somers
2020-09-25 07:41:51 UTC
Created attachment 110717 [details]
Screenshot
Screenshot of staff patron detail page
Created attachment 110718 [details] [review] Bug 26534: Add Font Awesome icon only for staff patrons In patron pages it would help to see an icon indicating that this is a staff patron (having permission to access staff interface). This may avoid wrong patron deletion or permission mistakes. I propose the shield Font Awesome icon and a grey background Patch also shows patron name and cardnumber aligned center instead of right. Patch adds to 'patroninfo' div a class 'staffgrade' or 'normalgrade' to allow more styling depending on patron beeing staff or not. Test plan: Compare display between a patron having staff interface access and one without permissions. Created attachment 110738 [details] [review] Bug 26534: Add Font Awesome icon only for staff patrons In patron pages it would help to see an icon indicating that this is a staff patron (having permission to access staff interface). This may avoid wrong patron deletion or permission mistakes. I propose the shield Font Awesome icon and a grey background Patch also shows patron name and cardnumber aligned center instead of right. Patch adds to 'patroninfo' div a class 'staffgrade' or 'normalgrade' to allow more styling depending on patron beeing staff or not. Test plan: Compare display between a patron having staff interface access and one without permissions. Signed-off-by: David Nind <david@davidnind.com> Created attachment 110739 [details] [review] Bug 26534: Add Font Awesome icon only for staff patrons In patron pages it would help to see an icon indicating that this is a staff patron (having permission to access staff interface). This may avoid wrong patron deletion or permission mistakes. I propose the shield Font Awesome icon and a grey background Patch also shows patron name and cardnumber aligned center instead of right. Patch adds to 'patroninfo' div a class 'staffgrade' or 'normalgrade' to allow more styling depending on patron beeing staff or not. Test plan: Compare display between a patron having staff interface access and one without permissions. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Nice enhancement, Passing QA + .staffgrade {
h5 {
background-color: #EEE;
}
}
This rule doesn't work as I think it is intended. If the staff name heading is to have a background color it should be written as:
&.staffgrade {
h5 {
background-color: #EEE;
}
}
Created attachment 110749 [details] [review] Bug 26534: (QA follow-up) Fix CSS declaration Created attachment 110750 [details] [review] Bug 26534: (follow-up) Only center when using patronimages I felt the change to always center the user title worked well when patronimages was enabled, but not so well when it was not (it looked strange without a background for non-staff users). Setting back to Signed off to get a second QA opinion as I missed the CSS issue first time around :(. I've corrected the CSS issue with a QA followup, but it also brought my attention to how the centered names looked when patronimages were not in use.. I felt they didn't look right in that case so have added a followup which should be looked at distinctly. Created attachment 110753 [details] [review] Bug 26534: (follow-up) Only center when using patronimages I felt the change to always center the user title worked well when patronimages was enabled, but not so well when it was not (it looked strange without a background for non-staff users). Created attachment 110754 [details] [review] Bug 26534: (follow-up) Only center when using patronimages I felt the change to always center the user title worked well when patronimages was enabled, but not so well when it was not (it looked strange without a background for non-staff users). Discussed in IRC and followup cleaned as per discussion.. Setting back to PQA as we seemed happy as a group :) Maybe avoid duplicated code this :
[% IF ( patronimages ) %]
<h5 class="text-center">
[% ELSE %]
<h5>
[% END %]
Fridolin, "grade" does not sound appropriate, could you use is_superlibrarian for instance? (In reply to Jonathan Druart from comment #14) > Fridolin, "grade" does not sound appropriate, could you use > is_superlibrarian for instance? would actually be "is_staff" in this context :) (In reply to Jonathan Druart from comment #15) > (In reply to Jonathan Druart from comment #14) > > Fridolin, "grade" does not sound appropriate, could you use > > is_superlibrarian for instance? > > would actually be "is_staff" in this context :) Yep ok no problem. (In reply to Fridolin SOMERS from comment #13) > Maybe avoid duplicated code this : > > [% IF ( patronimages ) %] > <h5 class="text-center"> > [% ELSE %] > <h5> > [% END %] Bah forget it. Created attachment 110960 [details] [review] Bug 26534: Replace 'grade' by 'is-patron' and 'patron-type' Looks great now, needs a final signoff ? (In reply to Fridolin SOMERS from comment #17) > (In reply to Fridolin SOMERS from comment #13) > > Maybe avoid duplicated code this : > > > > [% IF ( patronimages ) %] > > <h5 class="text-center"> > > [% ELSE %] > > <h5> > > [% END %] > > Bah forget it. Sorry, it wasn't a bad idea.. I just hadn't gotten around to it..I'm happy with it either way really myself. (In reply to Martin Renvoize from comment #20) > (In reply to Fridolin SOMERS from comment #17) > > (In reply to Fridolin SOMERS from comment #13) > > > Maybe avoid duplicated code this : > > > > > > [% IF ( patronimages ) %] > > > <h5 class="text-center"> > > > [% ELSE %] > > > <h5> > > > [% END %] > > > > Bah forget it. > > Sorry, it wasn't a bad idea.. I just hadn't gotten around to it..I'm happy > with it either way really myself. Ah OK super, i patch :D Created attachment 110972 [details] [review] Bug 26534: Use IF ELSE directly on h5 opening tag Something has gone wrong with these patches... Looks like maybe a merge error? They apply but I get an error. "Template process failed: file error - parse error - circ-menu.inc line 181: unexpected end of input" Created attachment 110995 [details] [review] Bug 26534: Use IF ELSE directly on h5 opening tag (In reply to Owen Leonard from comment #23) > Something has gone wrong with these patches... Looks like maybe a merge > error? They apply but I get an error. "Template process failed: file error - > parse error - circ-menu.inc line 181: unexpected end of input" Ah of course : IF, ELSE, was missing END ;) Created attachment 111185 [details] [review] Bug 26534: Add Font Awesome icon only for staff patrons In patron pages it would help to see an icon indicating that this is a staff patron (having permission to access staff interface). This may avoid wrong patron deletion or permission mistakes. I propose the shield Font Awesome icon and a grey background Patch also shows patron name and cardnumber aligned center instead of right. Patch adds to 'patroninfo' div a class 'staffgrade' or 'normalgrade' to allow more styling depending on patron beeing staff or not. Test plan: Compare display between a patron having staff interface access and one without permissions. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Created attachment 111186 [details] [review] Bug 26534: (QA follow-up) Fix CSS declaration Signed-off-by: David Nind <david@davidnind.com> Created attachment 111187 [details] [review] Bug 26534: (follow-up) Only center when using patronimages I felt the change to always center the user title worked well when patronimages was enabled, but not so well when it was not (it looked strange without a background for non-staff users). Signed-off-by: David Nind <david@davidnind.com> Created attachment 111188 [details] [review] Bug 26534: Replace 'grade' by 'is-patron' and 'patron-type' Signed-off-by: David Nind <david@davidnind.com> Created attachment 111189 [details] [review] Bug 26534: Use IF ELSE directly on h5 opening tag Signed-off-by: David Nind <david@davidnind.com> This is the test plan I used: 1. In a tab open a patron record for a staff member who has at least 'Staff access..." permissions. 2. In another tab enable the patronimages system preference: . Go to Home > Koha administration . Search for the patronimages system preference . Change to 'Allow' 3. In the same tab load the same patron as used in step 1. 4. Apply the patch. 5. Update the staff interface CSS: see https://wiki.koha-community.org/wiki/Working_with_SCSS_in_the_OPAC_and_staff_client 6. In yet another tab open the same patron record as in step 1. 7. It should look similar to the screenshot attached to the bug, with the shield icon, patron name and card number centred above the image with a light grey background. 8. Experiment with different browser widths to make sure things display nicely. 9. Turn off the patronimages system preference (Home > Koha administration > search for patronimages > change to 'Don't allow'). 10. Refresh the patron page. 11. It should display the shield icon, patron name, and card number left aligned with a light grey background. 12. Experiment with different browser widths to make sure things display nicely. 13. Sign off! Created attachment 111648 [details] [review] Bug 26534: Add Font Awesome icon only for staff patrons In patron pages it would help to see an icon indicating that this is a staff patron (having permission to access staff interface). This may avoid wrong patron deletion or permission mistakes. I propose the shield Font Awesome icon and a grey background Patch also shows patron name and cardnumber aligned center instead of right. Patch adds to 'patroninfo' div a class 'staffgrade' or 'normalgrade' to allow more styling depending on patron beeing staff or not. Test plan: Compare display between a patron having staff interface access and one without permissions. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 111649 [details] [review] Bug 26534: (QA follow-up) Fix CSS declaration Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 111650 [details] [review] Bug 26534: (follow-up) Only center when using patronimages I felt the change to always center the user title worked well when patronimages was enabled, but not so well when it was not (it looked strange without a background for non-staff users). Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 111651 [details] [review] Bug 26534: Replace 'grade' by 'is-patron' and 'patron-type' Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 111652 [details] [review] Bug 26534: Use IF ELSE directly on h5 opening tag Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> I really like this and think it would be nice in other spots like the patron search as well. But this is a solid start. Oh now I see that my code :
[% SET patron_is_staff = patron.has_permission({ 'catalogue' => 1 }) %]
Could have used existing var [% IF ( CAN_user_catalogue ) %].
Do I change ?
(In reply to Fridolin SOMERS from comment #38) > Oh now I see that my code : > [% SET patron_is_staff = patron.has_permission({ 'catalogue' => 1 }) %] > > Could have used existing var [% IF ( CAN_user_catalogue ) %]. > > Do I change ? tl:dr; Keep is as is. In this case, I actually like the addition of the patron_is_staff variable as it makes it clear we're not so much looking for 'cataglogue' as we're trying to just distinguish if the user is staff or not (In the future, we might eventually add role based access control).. also, CAN_user_catalogue is at the currently logged in user level, rather than on the patron we're interested in.. isn't it. > CAN_user_catalogue is at the currently logged in user
Ooooo crap sure, forget my comment ;)
Created attachment 111685 [details] [review] Bug 26687: Add color to Font Awesome icon for superlibrarian patrons Bug 26534 adds a shield Font Awesome icon to help identify staff patrons. I propose to color this icon when patron is a superlibrarian. Patch adds 'is-superlibrarian' class that may be used for more CSS tuning. Test plan: 1) Update staff CSS (yarn build) 2) Compare display between a patron A beeing superlibarian and a patron B having only staff interface access 3) Patron B has unchanged black shield icon 4) Patron A has red shield icon 5) Check a patron without permissions has no shield icon Oups wrong bug in git-bz (In reply to Katrin Fischer from comment #37) > I really like this and think it would be nice in other spots like the patron > search as well. But this is a solid start. I'll try to work on that. I've created Bug 26687 to color icon for superlibrarians B-) Isn't it weird to have a background colour only for staff users? Isn't the icon enough? (In reply to Jonathan Druart from comment #44) > Isn't it weird to have a background colour only for staff users? Isn't the > icon enough? I think it makes it stand out a little more than only the small icon - I liked it, but opinions may vary. Pushed to master for 20.11, thanks to everybody involved! (In reply to Jonathan Druart from comment #44) > Isn't it weird to have a background colour only for staff users? Isn't the > icon enough? Class 'is-not-staff' on 'patroninfo' div allows an easy CSS tunning depending on staff/non-staff Thanks a lot everyone. This code is my little baby, will feel proud each time I see the little shield :D enhancement will not be backported to 20.05.x |