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

[WIP] begin hyperlink rewrite #1137

Merged
merged 8 commits into from
Sep 18, 2024
Merged

[WIP] begin hyperlink rewrite #1137

merged 8 commits into from
Sep 18, 2024

Conversation

JanMarvin
Copy link
Owner

@JanMarvin JanMarvin commented Sep 15, 2024

  1. do not remove hyperlinks from wb$worksheet_rels
  2. do not replace wb$worksheets[[sheet]]$hyperlinks with hyperlink objects
  3. save what is available
  • fix commented tests
  • provide function to write native hyperlinks (additional PR)
  • provide function to modify/remove hyperlinks (additional PR)
  • skip adding hyperlinks to cells that already contain a hyperlink I leave that for the users

1) do not remove hyperlinks from `wb$worksheet_rels`
2) do not replace `wb$worksheets[[sheet]]$hyperlinks` with hyperlink objects
3) save what is available
@JanMarvin
Copy link
Owner Author

@olivroy , I would like to know what you think about wb_add_hyperlink(). It's a newly introduced wrapper to create what ooxml calls “shared hyperlinks”. These “shared hyperlinks” are basically “builtins” for =HYPERLINK(). The hyperlink formula is what we currently use to create hyperlinks. I added wb_add_hyperlink() similar to wb_add_formula() so that the function writes the data to the cell instead of just writing an overlay.

But I'm not sure if I'm really happy with this approach, as it bundles wb_add_data_table() and wb_add_data() internally and I'm not really sure if I want that. Especially since this is related to wb_dims() and the input and output dims might be different.

On the pro side ... it works as expected, which is nicer than before, although at the moment the references are not yet shared. This would require identifying unique strings in the input and matching them.

@olivroy
Copy link
Collaborator

olivroy commented Sep 16, 2024

I will have to take a deeper look, but first thing, having x before dims would be better for consistency with other methods.

I am in favour here I think as I had difficulty using hyperlink. I remember having opened a PR adding an example on how to do it.

@JanMarvin
Copy link
Owner Author

Thanks, it’s still work in progress. The order should be cleaned up, I agree.

@JanMarvin
Copy link
Owner Author

I have taken another approach now. There was to much going on in the first attempt. Now it is required to write the data into the sheet and pass a valid dimension to wb_add_hyperlink(). Only thing, I'm not yet happy with, is the selection of dims with col_names = TRUE, in case someone want's to use the column name with target or tooltip.

Probably in most cases people want to create hyperlinks not for columns, but for vectors/dimensions and it would be possible to switch to col_names = FALSE and if required, people can still enable it (or use A1 notion column names).

@olivroy
Copy link
Collaborator

olivroy commented Sep 18, 2024

Ok, yeah, it changes a bit the internal structure of the wbWorkbook object? Maybe it requires a clear explanation for people who may have relied on other workarounds till now to create / access hyperlinks.

@JanMarvin
Copy link
Owner Author

As usual, I worked more on the implementation. Documentation is a secondary goal 😄

@JanMarvin
Copy link
Owner Author

Everything should be good to go. Added documentation and NEWS entries. I'm quite happy how it has turned out. It simplifies our code, less things to stumble over, and we can provide hyperlinks that are closer to what users expect. Also something that I can remove from my invisible to-do list.
I want to have a remove and/or edit function for hyperlinks, but that can wait for another PR. This one is already way to big for my personal preferences.

@olivroy
Copy link
Collaborator

olivroy commented Sep 18, 2024

Looks good! Great work

@JanMarvin JanMarvin merged commit b752ffb into main Sep 18, 2024
9 checks passed
@JanMarvin JanMarvin deleted the hyperlinks_rewrite branch September 18, 2024 23:10
@JanMarvin
Copy link
Owner Author

Thanks! There are still a few things to think about, but for now: 🎉

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