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

LWS-225: improve display of libraries in holdings panel #1093

Merged
merged 2 commits into from
Aug 23, 2024

Conversation

jesperengstrom
Copy link
Contributor

Description

Tickets involved

LWS-225

Solves

Display additional (distinguishing) library information in the holdings panel.

This could really be as easy as printing the qualifier alongside the name. But I tried a more cumbersome approach 😆
To stay consistent with how we do it elsewhere (libraries in the facets, for example) we now display decorated data here.

Summary of changes

  • lensAndFormat the libraries inside the functions that prepare data for the holdings panel: holdingsByInstanceId and holdersByType.

  • Break out new util method getHoldingsByType in /utils.

  • New types and some checks to get rid of ts-warnings in /fnurgel/page.svelte.

  • The decorated labels are ellipsed, but displayed multi-line when opened.

  • We now also use DecoratedData component in facets instead of printing the rendered str. This should give us more fine-grained control over the styling?

Copy link
Contributor

@olovy olovy left a comment

Choose a reason for hiding this comment

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

LGTM!

Perhaps something like this to start with?

		"Collection-qualifier-format": {
			"@id": "Collection-qualifier-format",
			"@type": "fresnel:Format",
			"fresnel:classFormatDomain": ["Collection"],
			"fresnel:propertyFormatDomain": ["qualifier"],
			"fresnel:propertyFormat": {
				"fresnel:contentBefore": ", ",
				"fresnel:contentFirst": "",
				"fresnel:contentAfter": "",
				"fresnel:contentLast": ""
			}
		},
		"Collection-sigel-format": {
			"@id": "Collection-sigel-format",
			"@type": "fresnel:Format",
			"fresnel:classFormatDomain": ["Collection"],
			"fresnel:propertyFormatDomain": ["sigel"],
			"fresnel:propertyFormat": {
				"fresnel:contentBefore": " (",
				"fresnel:contentFirst": "(",
				"fresnel:contentAfter": ")",
				"fresnel:contentLast": ")"
			}
		},

It would be nice if sigel was a bit lighter but couldn't get it to work right away with propertyStyle

@jesperengstrom
Copy link
Contributor Author

Perhaps something like this to start with?

Nice! Added that. We could target it with [data-property='sigel'] but i'll investigate a little. Would like to learn more about the workings of the formatters.

@jesperengstrom
Copy link
Contributor Author

If "fresnel:propertyStyle": ["someClass"] is set in this case, it's added to _style as expected, but then ignored by the DecoratedData component. It applies classes for objects with a @type (data-type), but not in other cases (i.e data-property).

It works by simply adding
class={getStyleClasses(data)}

...but there must be some reason for this distinction @johanbissemattsson ?

@olovy
Copy link
Contributor

olovy commented Aug 22, 2024

I think it is a bit unclear if fresnel:propertyStyle should apply to contentBefore/After.
I think it should? Otherwise the parentheses get a different style in this case.
But then the whole thing has to be wrapped in an element or e.g. block breaks?

Styling the sigel in the same way as inScheme (sao etc) might be nice and consistent?
And avoids the issue for now

Skärmbild från 2024-08-22 17-03-02

@jesperengstrom
Copy link
Contributor Author

I think it is a bit unclear if fresnel:propertyStyle should apply to contentBefore/After.
I think it should? Otherwise the parentheses get a different style in this case.

Yeah, it's the same thing with role that always bugged me a bit

Skärmavbild 2024-08-22 kl  17 28 02

But having the parentheses underlined looks weird... Don't know if there's a universal solution to this 🤔

@olovy
Copy link
Contributor

olovy commented Aug 23, 2024

I think it is a bit unclear if fresnel:propertyStyle should apply to contentBefore/After.
I think it should? Otherwise the parentheses get a different style in this case.

Yeah, it's the same thing with role that always bugged me a bit
Skärmavbild 2024-08-22 kl 17 28 02

But having the parentheses underlined looks weird... Don't know if there's a universal solution to this 🤔

I think valueStyle should be used to set the underline and propertyStyle to set the size/color of the whole role parenthesis

@johanbissemattsson
Copy link
Contributor

Awesome! LGTM! 🌟

But having the parentheses underlined looks weird... Don't know if there's a universal solution to this 🤔

I think valueStyle should be used to set the underline and propertyStyle to set the size/color of the whole role parenthesis

I think this is the closest to a sensible solution, so I also think fresnel:propertyStyle should preferably also be applied to contentBefore/contentAfter.

@jesperengstrom
Copy link
Contributor Author

Styling the sigel in the same way as inScheme (sao etc) might be nice and consistent?

A bit reluctant to do that, since (I just now discovered) the contrast is too low and violates WCAG (it's not automatically detected since they're all collapsed on page load).

Skärmavbild 2024-08-23 kl  11 04 46

Should I continue to pursue better styling control through formatters, or is it becoming out of scope?

@jesperengstrom
Copy link
Contributor Author

Merging this now. Will do a separate PR of some changes I made trying to improve the _style thing. We can try it out there.

@jesperengstrom jesperengstrom merged commit 8a8fb91 into develop Aug 23, 2024
1 check passed
@jesperengstrom jesperengstrom deleted the feature/LWS-225-display-holders branch August 23, 2024 11:02
@olovy
Copy link
Contributor

olovy commented Aug 23, 2024

A bit reluctant to do that, since (I just now discovered) the contrast is too low and violates WCAG

I suspected it did!

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