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

222 gridlines on selected sheets #228

Merged
merged 7 commits into from
Aug 7, 2024
Merged

Conversation

ldavies99
Copy link

@ldavies99 ldavies99 commented Oct 4, 2023

Added a parameter in write/produce_workbook to show or hide gridlines on the worksheets and another parameter to choose whether to include the gridlines on the cover worksheet.

@ldavies99 ldavies99 added the enhancement ✨ New feature or request label Oct 4, 2023
@ldavies99 ldavies99 self-assigned this Oct 4, 2023
@ellie-o ellie-o linked an issue Jun 27, 2024 that may be closed by this pull request
Copy link
Collaborator

@rowanhemsi rowanhemsi left a comment

Choose a reason for hiding this comment

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

Left one comment about parameter options but this otherwise looks good 😃
Please can you update the tests to also check the gridlines, or let me know if there are any problems writing tests for this

gptables/core/api.py Outdated Show resolved Hide resolved
gptables/core/api.py Outdated Show resolved Hide resolved
@rowanhemsi
Copy link
Collaborator

I think a future enhancement would be for users to be able to specify which sheets should have gridlines using a dictionary but I want to get this approach merged in first and leave that for the future

Copy link
Collaborator

@rowanhemsi rowanhemsi left a comment

Choose a reason for hiding this comment

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

Thanks for making those changes. Just to check - should the numbers be 0-2 or 1-3?

gptables/core/wrappers.py Outdated Show resolved Hide resolved
Comment on lines 76 to 79
if cover_gridlines:
ws = wb.add_worksheet(cover.cover_label, gridlines=gridlines)
else:
ws = wb.add_worksheet(cover.cover_label, gridlines=2)
ws = wb.add_worksheet(cover.cover_label, gridlines="hide_all")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think the if-else is necessary if we specify a default value in the function definition? Does this work if you just have ws = wb.add_worksheet(cover.cover_label, gridlines=gridlines) ?

Copy link
Author

Choose a reason for hiding this comment

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

The if-else is for when you want gridlines on the workbook but not on the cover sheet specifically.

@ldavies99 ldavies99 requested a review from rowanhemsi July 2, 2024 10:59
Copy link
Collaborator

@rowanhemsi rowanhemsi left a comment

Choose a reason for hiding this comment

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

API test now passing and the expected workbook looks reasonable to me, so happy for this to get merged in now

@ldavies99 ldavies99 merged commit 82e6785 into dev Aug 7, 2024
13 checks passed
@ldavies99 ldavies99 deleted the 222-gridlines-on-selected-sheets branch August 7, 2024 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ✨ New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gridlines on selected sheets
2 participants