-
Notifications
You must be signed in to change notification settings - Fork 36
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
PR for #85: Ideate new approach to formatting table/diagram structure in template #97
base: master
Are you sure you want to change the base?
Conversation
@snairdesai, confirming a successful run on a Mac machine. Some files don't match the ones on this branch. I will push what I have on my end, but feel free to revert/overwrite if needed. |
@snairdesai, I think the current README is too lengthy. My suggestion would be to remove or push down any details on context or extra information. As a user, the first section I want to see is a clear intuition of the input and output, concrete steps for implementation, and only relevant warnings. I would remove all the details on the spreadsheet linking and the description of the function arguments (this should be documented in the code itself). |
@jc-cisneros Sounds good, and I agree. I'll remove the details on linking and function arguments. In the PR, can you use the "suggestion" tag to delete any other lines you feel are irrelevant? I can review and we'll discuss your proposed changes. |
@jc-cisneros: Latest version for your comments pushed in this commit. |
Thanks @snairdesai! The README looks less cluttered now. Will test this on a Windows machine. |
@snairdesai, I got the following error on Windows (also note that you need to run the terminal as administrator to get the right permission level):
|
Thanks @jc-cisneros, that is helpful. I'll try testing this on the lab Windows and see if I can debug. |
Closes #85.
The purpose of this PR is to extend the
template
architecture to enable native Excel to PDF conversions. See the issue thread for additional details, and in particular #85 (comment) for the final proposal of the workflow. The core workhorse function isexport_excel_tables()
ingslab_make()
(find the development version here).@jc-cisneros has been assigned as a reviewer. To review this PR, please complete the following:
/extensions/excel_tables/README.md
to ensure they are clear and capture the essence of our proposed direction in Ideate new approach to formatting table/diagram structure in template #85.gslab_make
to ensure they are logical (see the relevant lines here). If preferred, you can also achieve this as part of the parallel PR ingslab_make
(see here).Thanks for the review @jc-cisneros!