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

ExternalHyperlink with multiple children #1120

Merged
merged 4 commits into from
Sep 1, 2021
Merged

ExternalHyperlink with multiple children #1120

merged 4 commits into from
Sep 1, 2021

Conversation

rowanc1
Copy link
Contributor

@rowanc1 rowanc1 commented Aug 27, 2021

I am trying to have multiple TextRun children be in the same ExternalHyperlink. The way that the API is designed, there is only a single child, however, I have modified and tested multiple children on a word doc locally, and the hyperlink could potentially be extended to take multiple TextRuns?

This PR is not backwards compatible, and more work/advice would be needed to get it into shape. Looking for input to see if it is worth adding something along these lines?

See also #1119.

@dolanmiu
Copy link
Owner

dolanmiu commented Sep 1, 2021

Sure thing, will have a look at this PR

Yes most likely it won't be compatiable, not a problem, I am planning for a v7 release soon, this can be part of it

Thanks!

@codecov-commenter
Copy link

codecov-commenter commented Sep 1, 2021

Codecov Report

Merging #1120 (243d735) into master (9efee18) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 243d735 differs from pull request most recent head 727e741. Consider uploading reports for the commit 727e741 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1120   +/-   ##
=======================================
  Coverage   99.30%   99.30%           
=======================================
  Files         313      313           
  Lines        3578     3579    +1     
  Branches      381      381           
=======================================
+ Hits         3553     3554    +1     
  Misses         24       24           
  Partials        1        1           
Impacted Files Coverage Δ
src/file/paragraph/links/hyperlink.ts 100.00% <100.00%> (ø)
src/file/paragraph/paragraph.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9efee18...727e741. Read the comment docs.

@rowanc1
Copy link
Contributor Author

rowanc1 commented Sep 1, 2021

Fantastic, let me know if I can do anything else here! Great library. :)

@dolanmiu
Copy link
Owner

dolanmiu commented Sep 1, 2021

Can you update demo/35-hyperlinks.ts?

It is failing

@rowanc1
Copy link
Contributor Author

rowanc1 commented Sep 1, 2021

I have updated the demo and added an example of multi-children link.

Demo 35:
image

It looks like there is actually documentation for this feature, but it isn't linked into the table of contents? Also, it might be out of date? I could probably take a stab at updating that?

@dolanmiu
Copy link
Owner

dolanmiu commented Sep 1, 2021

Sure yes, if you can update that documentation, that would be doubly appreciated!

@rowanc1
Copy link
Contributor Author

rowanc1 commented Sep 1, 2021

Nice will make me learn about internal hyperlinks now ... :). Will get this done in the next 30min or so.

@rowanc1
Copy link
Contributor Author

rowanc1 commented Sep 1, 2021

I think that there is a similar capability for internal hyperlinks, if we are making a backwards incompatible change for v7, is it also worth updating that now? I haven't confirmed that this actually works, but I suspect it will. That is changing:

new InternalHyperlink({
    child: new TextRun({
        text: "Anchor Text",
        style: "Hyperlink",
    }),
    anchor: "myAnchorId",
})

To:

new InternalHyperlink({
    children: [
      new TextRun({
          text: "Anchor Text",
          style: "Hyperlink",
      }),
      new TextRun({
          text: "with style",
          bold: true,
          style: "Hyperlink",
      }),
    ],
    anchor: "myAnchorId",
})

Seems like we should from writing the docs ...!? Let me know.

@dolanmiu
Copy link
Owner

dolanmiu commented Sep 1, 2021

Yes updating that would also be good (but test afterwards)

@rowanc1
Copy link
Contributor Author

rowanc1 commented Sep 1, 2021

Sounds good. Tests seem to be working locally, will update this and demo 21 as well. Thanks for the fast response!

@dolanmiu
Copy link
Owner

dolanmiu commented Sep 1, 2021

Perfect!

@rowanc1
Copy link
Contributor Author

rowanc1 commented Sep 1, 2021

Passing locally, demos 21 & 35 updated. Docs match the style of docs online and added a link in the contents (after bullet points).

image

@rowanc1 rowanc1 marked this pull request as ready for review September 1, 2021 01:57
@rowanc1
Copy link
Contributor Author

rowanc1 commented Sep 1, 2021

Let me know if there is anything else to do on this. Off for my eve, but can pick stuff up tomorrow.

@dolanmiu dolanmiu merged commit 90bb670 into dolanmiu:master Sep 1, 2021
@dolanmiu
Copy link
Owner

dolanmiu commented Sep 1, 2021

Merged, looks good!

@rowanc1 rowanc1 deleted the feat/children-hyperlink branch September 1, 2021 03:42
@rowanc1
Copy link
Contributor Author

rowanc1 commented Sep 1, 2021

Thanks! Looking forward to this being released! 🚀

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.

None yet

3 participants