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

records/macros/detail.html: Allow funding entry with award number only #2912

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

karkraeg
Copy link
Member

@karkraeg karkraeg commented Nov 15, 2024

❤️ Thank you for your contribution!

Description

In order to display funding entries with only an award number after inveniosoftware/invenio-vocabularies#429 has been merged this PR changes the display logic.

Checklist

Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:

Reminder

By using GitHub, you have already agreed to the GitHub’s Terms of Service including that:

  1. You license your contribution under the same terms as the current repository’s license.
  2. You agree that you have the right to license your contribution under the current repository’s license.

Copy link
Contributor

@tmorrell tmorrell left a comment

Choose a reason for hiding this comment

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

I'd lean toward putting the title first and simplifying the template.

@karkraeg
Copy link
Member Author

Funder + Number

image

Two funding entries, second one with URL

image

No award number

image

Award number + title

image

Only award number

image

Just a funder name

image

@karkraeg
Copy link
Member Author

Hi @tmorrell , is this the way you were thinking?

@tmorrell
Copy link
Contributor

Could you add one or two screenshots?

@wgresshoff
Copy link

wgresshoff commented Nov 22, 2024

Perhaps the wrong place here (surely the wrong place here!) but it's just a thought: did someone bother to look if the change has implications on OAI-PMH output? If the transformers rely on number AND title instead of number OR title?!

@tmorrell
Copy link
Contributor

@wgresshoff Funding isn't serialized in the default dc oai output. The DataCite output should be alright assuming I understand how missing is working. @karkraeg if you've got a test instance up it would be worth running through the export formats on an example record and making sure they don't break.

</span>
{%- endif -%}
{%- if item.award.number -%}
<span class="ui mini basic ml-0 label {% if item.award.title_l10n %}mr-5{% endif %}" id="number-label-{{ index }}">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<span class="ui mini basic ml-0 label {% if item.award.title_l10n %}mr-5{% endif %}" id="number-label-{{ index }}">
<span class="ui mini basic label {% if not item.award.title_l10n %}ml-0{% endif %}" id="number-label-{{ index }}">

@tmorrell
Copy link
Contributor

The serialized formats are fine. Since inveniosoftware/invenio-vocabularies#429 has been released it would be great to get this change merged so the award number shows up consistently.

There seems to be a bit of a spacing issue around the award number:
Screenshot 2024-12-16 at 11 42 43 AM

I've just made a suggestion that re-works the spacing. I think we should merge after that change is added, unless folks want to do more re-designing.

Here is the UI after the change:
Screenshot 2024-12-16 at 12 08 55 PM
Screenshot 2024-12-16 at 12 09 28 PM
Screenshot 2024-12-16 at 12 10 02 PM
Screenshot 2024-12-16 at 12 10 28 PM
Screenshot 2024-12-16 at 12 11 02 PM

@karkraeg
Copy link
Member Author

karkraeg commented Dec 20, 2024

The serialized formats are fine. Since inveniosoftware/invenio-vocabularies#429 has been released it would be great to get this change merged so the award number shows up consistently.

Sorry Tom that I missed that... Looks nice this way, I'd opt for merging this.

@tmorrell
Copy link
Contributor

Could you make the change #2912 (comment) in your fork? Then I should be able to merge it.

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