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

OO: Library Lending #1031

Merged
merged 3 commits into from
May 3, 2020
Merged

OO: Library Lending #1031

merged 3 commits into from
May 3, 2020

Conversation

jimbo8098
Copy link
Contributor

Adding OOified DataTable to the Library Lending page. There are two functional differences here:

  1. Removing number column on the far left. I don't see a reason for it being there.
  2. The row is NOT red if the item is on loan. I'm not sure if the current DataTable would support such an "optional" CSS class. If there is, lemme know and I'll update it.

A screenshot detailing the difference is below. The new table is above the old one here.

image

@jimbo8098
Copy link
Contributor Author

I could maybe look at re-adding the Number column if it's helpful but I'd need to do so in SQL since I don't think there's a way to do that on the current DataTable class.

@rossdotparker
Copy link
Member

Hi Jim, thanks for this : ) The ID feild is definitely useful, especially as for many cases it is not numeric, but rather alphanumeric (e.g. my laptop is MBP00319).

For the row colouring, check out lines 84-87 of the file linked below, which use an anonymous function and a CSS class (error) to colour a row red:

https://github.com/GibbonEdu/module-masteryTranscript/blob/master/Mastery%20Transcript/credits_manage.php

I wonder if the warning class (orange) is better for loans. Perhaps overdue loans could be red?

Cheers,

Ross

@jimbo8098
Copy link
Contributor Author

jimbo8098 commented May 1, 2020 via email

@jimbo8098
Copy link
Contributor Author

Added new colouring now so when the item is pastDue, it will show using the "error" class. When it is on loan, as suggested, it will show using the "warning" class. All files committed here have been run through codesniffer to use PSR2 formatting.

@rossdotparker
Copy link
Member

Hi Jim, thanks for clarifying, and you are right, the number/count/# field is superfluous once the table is refactored, as the refactoring shows the total record count at the top, and I believe the count was there for similar ends.

@rossdotparker
Copy link
Member

I'll leave this with @SKuipers to review and merge in due course. Thanks for continuing to plug away at this : )

Copy link
Member

@SKuipers SKuipers left a comment

Choose a reason for hiding this comment

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

Another HTML table bites the dust! Thanks Jim 😄 I'm going to merge this in and test it.

As a note (for future refactorings) we tend to use full table names rather than shortening them to two and three letter aliases. It can certainly make the query look longer, but I think it does improve the readability by reducing any ambiguity. There are a lot of tables, for example gp could easily refer to gibbonPerson, Payment, Permission, etc.

@SKuipers SKuipers merged commit 0245066 into GibbonEdu:v20.0.00 May 3, 2020
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.

3 participants