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

feat: add wrapperTemplate option #348

Merged
merged 2 commits into from
Apr 26, 2023
Merged

feat: add wrapperTemplate option #348

merged 2 commits into from
Apr 26, 2023

Conversation

jdalrymple
Copy link
Contributor

@jdalrymple jdalrymple commented Dec 23, 2022

What: Adding support for a wrapper template that allows for a minimalist version of the contributors log. See original issue

Why: Modified the wrapping template from the defaulted 'table' to what ever the user supplies in their configuration

How: A simple if during the generation of the list wrapping. For a working sample, check this repo. After running the install, you may need to run yarn build from within the node_modules/all-contributors-cli directory to have access to the cli commands.

Checklist:

  • Documentation
  • Tests
  • Ready to be merged
  • Added myself to contributors table

@jdalrymple jdalrymple requested a review from a team as a code owner December 23, 2022 22:05
@jdalrymple jdalrymple changed the title Add support for wrapper template WIP: Add support for wrapper template Dec 23, 2022
@jdalrymple
Copy link
Contributor Author

Posted the PR to look for any feedback while i fill in the rest of the checklist!

@jdalrymple
Copy link
Contributor Author

Where are the docs located? 🤔

@jdalrymple jdalrymple changed the title WIP: Add support for wrapper template Add support for wrapper template Dec 23, 2022
@tenshiAMD
Copy link
Member

Where are the docs located? 🤔

@jdalrymple docs are in https://github.com/all-contributors/all-contributors.

Copy link
Member

@tenshiAMD tenshiAMD left a comment

Choose a reason for hiding this comment

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

@jdalrymple Thanks for this contribution. Please provide a working example for this one so we can compare. Also, I'm unsure about using other parent tags besides <table>. We also need to check the browser compatiblity.

@jdalrymple
Copy link
Contributor Author

Can do! The primary reason for the option to change the wrapper tag is because of the styling limitations within markdown linked to the table tag. For example in that issue, the desire to have a minimalist list of contributors such as:

image

Can only be accomplished with either styling (which isnt supported in github markdown) or changing the tag used to wrap the content. I was able to accomplish this example with p tag instead of the table tag, however knowing that things should not be hardcoded, i figured using a configurable option would be the way to go. Ill post up a sample repo ASAP!

@jdalrymple
Copy link
Contributor Author

jdalrymple commented Dec 24, 2022

The only caveat** Is the number of contributors per line. Need to think of a good way to support that with different wrapper tags I added a break tag between tr tags when using a custom wrapping tag to support the number of contributors per line configuration.

@jdalrymple
Copy link
Contributor Author

Is there anything else that i need to address for this to be merged?

Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

Thank you for the pull request! Sorry for the delay!

@gr2m gr2m changed the title Add support for wrapper template feat: add wrapperTemplate option Apr 26, 2023
@gr2m gr2m merged commit 2c07af8 into all-contributors:master Apr 26, 2023
@gr2m
Copy link
Contributor

gr2m commented Apr 26, 2023

@all-contributors add @jdalrymple for code and test

@allcontributors
Copy link
Contributor

@gr2m

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

@all-contributors-release-bot
Copy link
Member

🎉 This PR is included in version 6.25.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants