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

Fix Packages Page Listing Missing Maintainer #172

Merged

Conversation

bbulpett
Copy link
Contributor

What was done


Addressing issue #169

  • Updated the packages.yml file with latest package data
  • Modified package-grid.html template to conditionally display contributor names in the following manner
    -- If the all_current_maintainers value is present for the package in the Yaml file, loop through the maintainers' names (with fallback to GitHub user names) and display in a comma-delimited format
    -- Otherwise, display the submitting author's name (with fallback to GitHub user name)
  • Create a link on the displayed user name to each maintainer's public GitHub profile
  • Removed extraneous packages.yml and contributors.yml files from application root

See the following screen clipping example:
Screen Shot 2023-04-27 at 10 11 53 PM

How to test


  • Open the python-packages template in the browser
  • Note that packages with multiple contributors now display each contributor in a comma-separated list
  • Note that packages with missing "author" names now display the GitHub username of the submitting author
  • Observe that, by clicking author user names, the public GitHub profile page is opened (in a new tab/window) for that maintainer

Special thanks to the team at PyCon 2023 for making this possible, with their help and encouragement! Thank you especially to @lwasser.

@lwasser
Copy link
Member

lwasser commented May 4, 2023

@all-contributors please add @bbulpett for review, code, design

@allcontributors
Copy link
Contributor

@lwasser

I've put up a pull request to add @bbulpett! 🎉

@lwasser
Copy link
Member

lwasser commented May 4, 2023

Ok @bbulpett this is so great! many thanks. I noticed that there is an extra space before each comma. it's weird. I think it's because of where the conditional for the ending comma sits.

Screen Shot 2023-05-04 at 11 06 10 AM

if i do this

{% if maintainer.name %}
     {{ maintainer.name }}{% if forloop.last == false %}, {% endif %}
{% else %}
     {{ maintainer.github_username }}{% if forloop.last == false %}, {% endif %}
{% endif %}

it removes the extra space but now the code is a bit messier as there is repetition via the conditional statement adding the comma. If we reorganize the yaml file to have a list of authors we could also do this:

<meta name="keywords" content="{{page.tags | join: ', '}}"/>

but i like what you did as it links to the users github and allows us to have missing information and still work.

so i'm inclined to just add the comma to each part of the conditional to fix the space. but open to another approach if you know of one?

let me know what you think. we can merge this as is with the small fix that is a bit redundant if that is the best option!

@bbulpett
Copy link
Contributor Author

bbulpett commented May 5, 2023

let me know what you think. we can merge this as is with the small fix that is a bit redundant if that is the best option!

Hi @lwasser - nice catch there! I didn't even notice that in my sanity check. Yes, I think merging your fix is the right course of action here. Minor redundancy in the view layer isn't always a bad thing, if it gets the job done 🙂

Let me know if there's anything actionable on my end and I'll take care of it quickly. Also, I just noticed the CI check failures. If that's something I should address, just say the word and I'll be happy to.

Thanks again!

@lwasser
Copy link
Member

lwasser commented May 5, 2023

@bbulpett many many thanks! it was a very tiny change so i just implemented it! i'll merge this once pre-commit finishes running!! many thanks again for this fix / enhancement!!

@lwasser
Copy link
Member

lwasser commented May 5, 2023

pre-commit.ci autofix

@lwasser
Copy link
Member

lwasser commented May 5, 2023

ok the point of failure here is still the moving pandas url's. one if fixed in main, the other is in progress to be fixed. so we can merge this! thank you again for the PR!!

@lwasser lwasser merged commit ab7bfc5 into pyOpenSci:main May 5, 2023
1 check passed
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.

None yet

2 participants