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.

Sign up for GitHub

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

Jump to bottom

Add repeat table headers #105

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

Add repeat table headers #105

wants to merge 2 commits into from

Conversation

AliceVerner
Copy link

Repeat table headers

@jdugan
Copy link
Contributor

jdugan commented Sep 4, 2018

Hei, Alice.

Thanks for the thoughtful PR. A few thoughts:

  • I wonder if the boolean is necessary. Said differently, if the repeating header rows count parameter defaults to zero (which we need to do to avoid a backwards-incompatible change), it would already be truthy/falsey. I was thinking the interface might be simpler to either declare a certain number of repeating rows and get that behaviour, or declare nothing and get no repeating behaviour.
  • Please add corresponding tests to the table model spec.
  • Please update the README so other users can learn about the feature.

Thanks!

@AliceVerner
Copy link
Author

Hi, John!
I implemented your comments and fixed code.
Thank you for your advices!

@jdugan
Copy link
Contributor

jdugan commented Sep 21, 2018

Awesome! I tend to work on issues and PRs once a month at a local meetup, so I'll do a final review then and get everything integrated into an official release.

I'm super fussy about naming so I may make a few cosmetic alterations to your code, but I'll ping you here either way so you can update your Gemfile and, if necessary, do a fast find-and-replace on your caracal views.

Thanks for contributing!

@AliceVerner
Copy link
Author

Any news around this @jdugan? Something I can do?

goulvench added a commit to goulvench/caracal that referenced this pull request Mar 18, 2020
@goulvench
Merge pull request urvin-compliance#105 from AliceVerner:repeat_headers
f3029a5 
Add repeat table headers

# Conflicts:
#	lib/caracal/core/models/table_model.rb
#	lib/caracal/renderers/document_renderer.rb
@shubham-yhakur-96
Copy link

Hello @AliceVerner

Thanks for sharing this,

Why your PR did not merge it will help everyone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AliceVerner @jdugan @shubham-yhakur-96