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

Recommend Us views and urls #81

Merged
merged 3 commits into from
Oct 31, 2024
Merged

Recommend Us views and urls #81

merged 3 commits into from
Oct 31, 2024

Conversation

joemull
Copy link
Member

@joemull joemull commented Oct 25, 2024

Requires openlibhums/hourglass#403.

Closes #74.

Notes:

  • To test, go to /plugins/supporters/recommend-us/.
  • Blue backgrounds are not what I'd prefer for this but it is what we need for forms right now, until we create a form light mode (would be a bit involved due to accessibility requirements around forms). Up to Rose whether this is OK for the MVP or she'd prefer to wait a bit longer for a light-theme form.
  • There is a known a11y contrast issue with the numbers on red and orange, but we plan to fix that separately for this component.
  • Artwork is not included at the top of the page yet. Will work on this later this week on the Hourglass branch.

image

@joemull joemull marked this pull request as draft October 25, 2024 08:50
@joemull joemull requested a review from ajrbyers October 28, 2024 18:55
@joemull joemull marked this pull request as ready for review October 28, 2024 18:55
@rhb123
Copy link

rhb123 commented Oct 29, 2024

Thanks for this @joemull - looks great! It would be good to get access to the test version asap so I can go through and start editing the copy accordingly, if you could let me know when this becomes possible? Thanks again - amazing work

Copy link
Member

@ajrbyers ajrbyers left a comment

Choose a reason for hiding this comment

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

This works really well.

@ajrbyers ajrbyers requested a review from mauromsl October 29, 2024 11:15
@ajrbyers ajrbyers assigned mauromsl and unassigned ajrbyers Oct 29, 2024
Copy link
Member

@mauromsl mauromsl left a comment

Choose a reason for hiding this comment

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

nice one @joemull !

I've added a few suggestions inline

views.py Outdated Show resolved Hide resolved
views.py Outdated Show resolved Hide resolved
views.py Outdated

logger = get_logger(__name__)


def hourglass(template):
Copy link
Member

Choose a reason for hiding this comment

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

can we use a name that better describes the behaviour of this function? we probably want to reserve the name hourglass for an eventual module, rather than for a utility function here

Personally, I would avoid coupling this bit of code with the "hourglass" theme and instead handle the missing template in a more generic fashion, thought I believe it would be too little too short at this point.

},
{
"group": {
"name": "plugin:consortial_billing"
Copy link
Member

Choose a reason for hiding this comment

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

I understand why you'd want to use the plugin naming scheme for the group, but bear in mind these will not be seen as email templates elsewhere in Janeway (e.g. email template manager). setting the group as "email" is the only way to achieve that at the moment.

If you wanted this behaviour, every email setting will also need a "email_subject" group member as a counterpart.

views.py Outdated
Comment on lines 579 to 580
submission_models.Article,
pk=article_pk,
Copy link
Member

Choose a reason for hiding this comment

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

should use the PublishedArticles manager, as you've done previously

views.py Outdated
Comment on lines 589 to 592
'journal': get_object_or_404(
Journal,
pk=journal_pk,
)
Copy link
Member

Choose a reason for hiding this comment

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

I think we also want the filter you used previously:
journal__in=request.press.public_journals,

@mauromsl mauromsl assigned joemull and unassigned mauromsl Oct 29, 2024
@joemull joemull requested a review from mauromsl October 30, 2024 14:49
@joemull joemull assigned mauromsl and unassigned joemull Oct 30, 2024
@mauromsl mauromsl merged commit 19c8c03 into main Oct 31, 2024
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.

As an author I want to generate an email to my librarian
4 participants