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

Add support for a description column in fines. #3171

Merged
merged 4 commits into from
Oct 23, 2023

Conversation

EreMaijala
Copy link
Contributor

@EreMaijala EreMaijala commented Oct 23, 2023

Includes driver support for Demo and SierraRest. SierraRest also removes some custom handling of types and adds optional mappings. Finnish and Swedish translations include a fix for duplicate column heading.

TODO:

Includes driver support for Demo and SierraRest. SierraRest also removes some custom handling of types and adds optional mappings. Finnish and Swedish translations include a fix for duplicate column heading.
@EreMaijala EreMaijala requested a review from demiankatz October 23, 2023 09:34
Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks, @EreMaijala -- one small question and one minor issue to fix.

module/VuFind/src/VuFind/ILS/Driver/Demo.php Show resolved Hide resolved
config/vufind/SierraRest.ini Show resolved Hide resolved
Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

The code looks great, and the output is also fine in most themes. However, this happens in bootprint3:

image

Might there be an easy CSS fix to reduce the odds of this happening? I'm not sure why the browser isn't deciding to make the "Fee" column a little smaller so that it doesn't have to wrap the "Description" heading.

@demiankatz
Copy link
Member

Lacking a smarter solution, we could also abbreviate the English translation to "Desc." -- but then I imagine we'll still see similar problems for other languages with headings of similar length.

@demiankatz
Copy link
Member

...or maybe we should just widen the default bootprint3 width, since it's feeling pretty cramped these days. If that's our best solution, it might be worth addressing in a separate PR and just living with this as-is for now.

@demiankatz demiankatz added this to the 10.0 milestone Oct 23, 2023
@EreMaijala
Copy link
Contributor Author

EreMaijala commented Oct 23, 2023

@demiankatz There are these rules in bootstrap.less that have been there since dawn of the file:

/* ------ Table wrapping to prevent horizontal overflow ------ */
.table {
  table-layout: fixed;
  word-wrap: break-word;
}

I understand that particularly letting words wrap in the middle helps fit the table to narrow screens as well, though I think we should try to avoid that since the result is quite horrible. The downside of the fixed layout is that it practically makes all columns equal width for a table with no explicit widths. I added a rule now to override this for the fines table on wide screens.

@EreMaijala
Copy link
Contributor Author

Also note that this is not really a new issue. Even without the description column we have some issues displaying the table in the minimum 768px width with dates wrapping in a not very nice way.

@EreMaijala
Copy link
Contributor Author

Example of how the headings wrap in Finnish:
kuva

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks, @EreMaijala, this is much better. I'm not sure in your screen shot why the statistics on the ILL link are overflowing the page, though; I'm not seeing the same behavior on my end.

In any case, I'll merge this as-is, but we should do follow-up work to improve our tables generally -- by adding more responsive styling and/or by eliminating tables where other HTML elements might be more useful and appropriate. I spoke to @crhallberg about this a little bit; perhaps he can make some proposals in a JIRA ticket or PR in the near future.

@demiankatz demiankatz merged commit 1b4c54e into vufind-org:dev Oct 23, 2023
6 checks passed
@demiankatz demiankatz deleted the dev-fines-description branch October 23, 2023 19:04
EreMaijala added a commit to NatLibFi/NDL-VuFind2 that referenced this pull request Oct 24, 2023
Includes driver support for Demo and SierraRest. SierraRest also removes some custom handling of types and adds optional mappings. Finnish and Swedish translations include a fix for duplicate column heading.
EreMaijala added a commit to NatLibFi/NDL-VuFind2 that referenced this pull request Oct 24, 2023
Includes driver support for Demo and SierraRest. SierraRest also removes some custom handling of types and adds optional mappings. Finnish and Swedish translations include a fix for duplicate column heading.
EreMaijala added a commit to NatLibFi/NDL-VuFind2 that referenced this pull request Oct 24, 2023
Includes driver support for Demo and SierraRest. SierraRest also removes some custom handling of types and adds optional mappings. Finnish and Swedish translations include a fix for duplicate column heading.
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.

2 participants