Summary: | Tag status does not show on multiple tag add | ||
---|---|---|---|
Product: | Koha | Reporter: | Fridolin Somers <fridolin.somers> |
Component: | OPAC | Assignee: | Fridolin Somers <fridolin.somers> |
Status: | CLOSED FIXED | QA Contact: | Jonathan Druart <jonathan.druart> |
Severity: | normal | ||
Priority: | P5 - low | CC: | chris, jcamins, jonathan.druart, mtj, paul.poulain, sophie.meynieux |
Version: | 3.10 | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | Medium patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Circulation function: | |||
Attachments: |
Proposed patch
Proposed patch (2) Proposed patch (3) [SIGNED-OFF] Bug 8660: Tag status does not show on multiple tag add Bug 8860 follow-up: Porting opac.css changes to cssr theme Proposed patch (rebased master) Proposed patch (follow-up) (rewritten) Bug 8660: follow-up: Porting changes to cssr theme |
Description
Fridolin Somers
2012-08-21 09:37:29 UTC
Created attachment 11750 [details] [review] Proposed patch Proposed patch adds a lot of corrections in tags edition in OPAC. Affects search results, basket, list and detail pages. I tried to uniform the code on those pages. Main correction : On search results / basket / list page, when adding tag on several records, a status appears on each selected record line. I used a border and yellow background. When clicking on button to end tag edition, the status of all records is initialized. Concerning sysprefs, they where missing in some places. I used TagsShowOnDetail and TagsInputOnDetail for detail page and TagsShowOnList and TagsInputOnList for other pages. In basket, the toolbar was existing twice in TT (for basket witt or without details). The code was the same so I moved it to be unique. I really like the improvements in this patch, but I also have a complaint: Whitespace changes in otherwise unchanged lines makes it difficult to track what is really different and what isn't. I would prefer that whitespace-only changes be submitted in a separate patch if they're really desirable. I see only one glitch in functionality that needs to be addressed: If I choose some items (in results, the cart, or a list) and add a tag, I see the "tag added" message by each one. If I then select some different items on that same list and add another tag, the "tag added" message on the original selection doesn't disappear altogether, it just loses its text, leaving an empty yellow box behind. It's not a big issue, but I would like to see a revision before I sign off. Thanks! (In reply to comment #2) > I really like the improvements in this patch, but I also have a complaint: > Whitespace changes in otherwise unchanged lines makes it difficult to track > what is really different and what isn't. I would prefer that whitespace-only > changes be submitted in a separate patch if they're really desirable. > I agree, speaking as a release maintainer, separate patches make my life (and anyone ever doing code maintenance in the future) a lot easier. Created attachment 11766 [details] [review] Proposed patch (2) (In reply to comment #2) > > I see only one glitch in functionality that needs to be addressed: > > If I choose some items (in results, the cart, or a list) and add a tag, I > see the "tag added" message by each one. If I then select some different > items on that same list and add another tag, the "tag added" message on the > original selection doesn't disappear altogether, it just loses its text, > leaving an empty yellow box behind. > > It's not a big issue, but I would like to see a revision before I sign off. > Thanks! Your right, corrected in new patch (a span with border-radius must be hidden when empty) Thanks a lot for testing. Created attachment 11767 [details] [review] Proposed patch (3) > Whitespace changes in otherwise unchanged lines makes it difficult to track > what is really different and what isn't. I would prefer that whitespace-only > changes be submitted in a separate patch if they're really desirable. I agree, I reduced in new patch the new line/whitespace changes. Thanks for your interest in this bug. I think it is important for OPAC users. Created attachment 11786 [details] [review] [SIGNED-OFF] Bug 8660: Tag status does not show on multiple tag add Signed-off-by: Owen Leonard <oleonard@myacpl.org> QA comment: * patch signed-off since more than 1 month, QAing it myself * template only changes, low side effect risk * koha-qa.pl is OK: ../qa/koha-qa.pl -v 2 -c 1 testing 1 commit(s) (applied to commit 813c744) * 8b19523 Bug 8660: Tag status does not show on multiple tag add koha-tmpl/opac-tmpl/prog/en/css/opac.css koha-tmpl/opac-tmpl/prog/en/includes/doc-head-close.inc koha-tmpl/opac-tmpl/prog/en/js/tags.js koha-tmpl/opac-tmpl/prog/en/modules/opac-basket.tt koha-tmpl/opac-tmpl/prog/en/modules/opac-detail.tt koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt koha-tmpl/opac-tmpl/prog/en/modules/opac-shelves.tt koha-tmpl/opac-tmpl/prog/en/modules/opac-suggestions.tt - koha-tmpl/opac-tmpl/prog/en/includes/doc-head-close.inc OK forbidden patterns OK tt_valid OK valid_template OK - koha-tmpl/opac-tmpl/prog/en/modules/opac-basket.tt OK forbidden patterns OK tt_valid OK valid_template OK - koha-tmpl/opac-tmpl/prog/en/modules/opac-detail.tt OK forbidden patterns OK tt_valid OK valid_template OK - koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt OK forbidden patterns OK tt_valid OK valid_template OK - koha-tmpl/opac-tmpl/prog/en/modules/opac-shelves.tt OK forbidden patterns OK tt_valid OK valid_template OK - koha-tmpl/opac-tmpl/prog/en/modules/opac-suggestions.tt OK forbidden patterns OK tt_valid OK valid_template OK * some css added to opac/prog, I added them in opac/ccsr as well Testing the feature (using the CCSR theme) * when I add tag to multiple biblios, I see the status on each line * when I add a tag to a given biblio, from the biblio list, It does not work anymore= clicking on "Add" does nothing (no message, no change, no javascript error) Created attachment 12642 [details] [review] Bug 8860 follow-up: Porting opac.css changes to cssr theme Created attachment 12905 [details] [review] Proposed patch (rebased master) Main patch rebased Created attachment 12906 [details] [review] Proposed patch (follow-up) (rewritten) Follow-up rewritten : Same modifications in opac.css. Added porting of modifications in doc-head-close.inc and tags.js (also corrects the comment : 8860 instead of 8660) Created attachment 13168 [details] [review] Bug 8660: follow-up: Porting changes to cssr theme Signed-off-by: Mason James <mtj@kohaaloha.com> (In reply to comment #12) > Created attachment 13168 [details] [review] > Bug 8660: follow-up: Porting changes to cssr theme > > Signed-off-by: Mason James <mtj@kohaaloha.com> i tested this patch with the CSSR theme, it works well :) String changes. Will hold for post-3.10.0. QA Comments: Great and clean patch. Note: I did not test the CCSR theme. Marked as Passed QA. This patch has been pushed to master. Pushed to 3.10.x will be in 3.10.1 Could it be also in 3.8.x ? Doesn't apply to 3.8.x if it could be rebased and signed off then yep :) |