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

Review threaded comment docs #765

Merged
merged 1 commit into from
Aug 24, 2023
Merged

Conversation

olivroy
Copy link
Collaborator

@olivroy olivroy commented Aug 24, 2023

So that a user who scan do not see wb_get_person() before.

image

Thought I'd do that before taking a look at the internals

Edit: works in Excel! but interestingly. threaded comments are hidden in Excel 365, another justification for the change of default value in #758

@JanMarvin
Copy link
Owner

Thanks for the heads up, could you add a visible argument to threaded comments? Guess that could be helpful.

@olivroy
Copy link
Collaborator Author

olivroy commented Aug 24, 2023

Not sure how it will respected however? Possibly after 1.0? cause atm, I have to hover over it (with create_comment() as internal) even though the internal still uses create_comment() rn. (about to change it in another PR.

I observed that openxlsx2 add threaded comment with my system default.

@JanMarvin
Copy link
Owner

Unless you have anything urgent, I'd like to close the development for 1.0 with this PR (and/or a second one with the visibilty argument). So that we can rest for the day and do a release tomorrow or early Saturday.

@JanMarvin
Copy link
Owner

I was thinking of the following

wb_add_... <- (wb, visible = FALSE) {

  wb$add_...(visible = visible) # calling create_comment(..., visible = visible)

}

@olivroy
Copy link
Collaborator Author

olivroy commented Aug 24, 2023

I have just 2 small left. I will be v quick!

@JanMarvin
Copy link
Owner

Alright, no rush. I just have a few things going on over the weekend will be a bit unavailable early next week. There is no hard deadline :)

@JanMarvin
Copy link
Owner

Ah, this can be merged? Looks fine, although I liked the previous user a bit more 😛

@olivroy
Copy link
Collaborator Author

olivroy commented Aug 24, 2023

can this be merged?

Yes

I liked the previous user a bit more

What do you mean? I don't mind tweaking!

My motivation is just that person can be many things if put out of context, could be wb creator author, but since this is very specific to threaded comments, I thought that for avoiding confusion to hide it a bit more.

@JanMarvin
Copy link
Owner

You switched openxlsx2 to something else "a person who likes ...", but that was not a serious comment. I don't really mind :)

@JanMarvin JanMarvin merged commit e29f46e into JanMarvin:main Aug 24, 2023
9 checks passed
@JanMarvin
Copy link
Owner

Thank you, merged!

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.

2 participants