👌 Use reference name by default for internal link cards#183
👌 Use reference name by default for internal link cards#183chrisjsewell merged 4 commits intoexecutablebooks:mainfrom
Conversation
For cards that link to internal references, we can use the associated name of the reference to supply the link text, so that the link isn't left blank if `link-alt` is not provided.
48b95cd to
47bb4a2
Compare
gabalafou
left a comment
There was a problem hiding this comment.
done self-reviewing
There was a problem hiding this comment.
I regenerated the test snippet snapshots using pytest --force-regen
docs/cards.md
Outdated
|
|
||
| The entire card can be clicked to navigate to <https://example.com>. | ||
|
|
||
| For **external** link cards, if you do not provide `link-alt`, the URL will be |
There was a problem hiding this comment.
I think it would be good to add tests for this behavior, i.e.:
- when card link is external and no link-alt is provided, use the URL provided by the link option
- when card link is external and link-alt is provided, use the link-alt text
- when card link is internal and no link-alt is provided, use the (section) name of the reference
- when card link is internal and link-alt is provided, use the link-alt text
- in all cases, apply the hide-link class
The current snapshots in this PR implicitly cover 2 and 3 but it would be good to have individual test lines that spell these conditions out.
I'm just not entirely sure how to go about it, since all of the current tests are regression snapshot tests.
I suppose I could go ahead and add snippets for conditions 1 (card-link-external-no-link-alt) and 4 (card-link-internal-with-link-alt), which would also imply 5.
Anyway, let me know what you would like me to do. Happy to add tests in whatever way seems best.
docs/cards.md
Outdated
| title for the link, you can provide `link-alt` and it will take precedence. | ||
| ::: | ||
|
|
||
| Note: you cannot add additional links to the clickable card, neither in the card |
There was a problem hiding this comment.
I added this because I tried adding a link inside the card body of the external clickable card to a page explaining why link text is important but clicking on the link took me to example.com 😆
chrisjsewell
left a comment
There was a problem hiding this comment.
cheers! I updated the docs/tests a little, but mainly all good
|
Congrats on your first merged pull request in this project! 🎉 |
|
Brilliant! A few observations, please let me know if you would like me to submit follow-ups for any of them:
Thanks! |
For cards that link to internal references, we can use the associated name of the reference to supply the link text, so that the link isn't left blank if
link-altis not provided.