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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

馃憣 Use reference name by default for internal link cards #183

Merged
merged 4 commits into from
May 22, 2024
Merged

馃憣 Use reference name by default for internal link cards #183

merged 4 commits into from
May 22, 2024

Conversation

gabalafou
Copy link
Contributor

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.

@gabalafou
馃憣 IMPROVE: Use reference name by default for internal link cards
47bb4a2 
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.
gabalafou
gabalafou commented Mar 21, 2024
View reviewed changes
Copy link
Contributor Author

@gabalafou gabalafou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done self-reviewing

sphinx_design/cards.py Show resolved Hide resolved
sphinx_design/cards.py Show resolved Hide resolved
tests/test_snippets/snippet_post_card-link.xml Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I regenerated the test snippet snapshots using pytest --force-regen

docs/snippets/myst/card-link.txt Show resolved Hide resolved
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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to add tests for this behavior, i.e.:

  1. when card link is external and no link-alt is provided, use the URL provided by the link option
  2. when card link is external and link-alt is provided, use the link-alt text
  3. when card link is internal and no link-alt is provided, use the (section) name of the reference
  4. when card link is internal and link-alt is provided, use the link-alt text
  5. 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
:::

Note: you cannot add additional links to the clickable card, neither in the card
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 馃槅

@gabalafou gabalafou mentioned this pull request Mar 21, 2024
Merged
@chrisjsewell chrisjsewell changed the title 馃憣 IMPROVE: Use reference name by default for internal link cards 馃憣 Use reference name by default for internal link cards May 22, 2024
chrisjsewell
chrisjsewell approved these changes May 22, 2024
View reviewed changes
Copy link
Member

@chrisjsewell chrisjsewell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cheers! I updated the docs/tests a little, but mainly all good

@chrisjsewell chrisjsewell merged commit e258b45 into executablebooks:main May 22, 2024
11 of 15 checks passed
@welcome Welcome
Copy link

welcome bot commented May 22, 2024

Congrats on your first merged pull request in this project! 馃帀
congrats

Thank you for contributing, we are very proud of you! 鉂わ笍

@chrisjsewell chrisjsewell mentioned this pull request May 22, 2024
Closed
@gabalafou gabalafou deleted the docs-link-alt branch May 22, 2024 17:23
@gabalafou
Copy link
Contributor Author

Brilliant!

A few observations, please let me know if you would like me to submit follow-ups for any of them:

  1. The code in the syntax dropdown does not exactly match the two card examples above it. Was that intended? (For comprehension, I find it very helpful when I consult docs if I can easily correlate syntax examples with rendered results, which is harder to do if there are unrendered blocks in the syntax view.)
  2. A distinction that I was trying to make got a bit lost in the docs restructure:
    • External link card? Use link-alt
    • Internal? Do NOT use link-alt
  3. The equivalency paragraphs were a bit hard for me to follow. I think I would find them easier to follow if you add "looks like" in the parentheses: `<https://example.com>` (looks like <https://example.com>)

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrisjsewell chrisjsewell chrisjsewell approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@gabalafou @chrisjsewell