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

[LiquidDoc] Prevent rendering of hover tooltip for snippets with no renderable content #729

Merged
merged 4 commits into from
Jan 27, 2025

Conversation

jamesmengo
Copy link
Contributor

@jamesmengo jamesmengo commented Jan 23, 2025

What are you adding in this PR?

{% render 'snippet' %}

Prevents rendering hover tooltip for snippets that:

  • do not exist
  • do not have renderable content.

Renderable content: Currently, this means that it requires a LiquidDoc definition with @param annotations, though this will soon expand to the description and @example as well

Before you deploy

  • This PR includes a new checks or changes the configuration of a check
    • I included a minor bump changeset
    • It's in the allChecks array in src/checks/index.ts
    • I ran yarn build and committed the updated configuration files
      • If applicable, I've updated the theme-app-extension.yml config
  • I included a minor bump changeset
  • My feature is backward compatible
  • I included a patch bump changeset

@jamesmengo jamesmengo changed the title [LiquidDoc] Return undefined for invalid snippet definitions Stop rendering hover toolbox Jan 23, 2025
@jamesmengo jamesmengo changed the title Stop rendering hover toolbox Prevent rendering of hover tooltip for nonexistent snippets Jan 23, 2025
@jamesmengo jamesmengo force-pushed the jm/prevent_hover_nonexistent_snippets branch from 6ad37f6 to 1f7a561 Compare January 23, 2025 02:09
@jamesmengo jamesmengo changed the title Prevent rendering of hover tooltip for nonexistent snippets Prevent rendering of hover tooltip for snippets with no renderable content Jan 23, 2025
@jamesmengo jamesmengo changed the title Prevent rendering of hover tooltip for snippets with no renderable content [LiquidDoc] Prevent rendering of hover tooltip for snippets with no renderable content Jan 23, 2025
@jamesmengo jamesmengo marked this pull request as ready for review January 23, 2025 02:26
@jamesmengo jamesmengo requested a review from aswamy January 23, 2025 02:27
@jamesmengo jamesmengo requested review from a team and removed request for aswamy January 23, 2025 02:27
@jamesmengo jamesmengo self-assigned this Jan 23, 2025
Copy link
Contributor Author

@charlespwd Branching off of this comment for snippets without a liquidDoc definition, what is some valuable content we can render here?

Is there value in rendering the below?

image.png

@charlespwd
Copy link
Collaborator

I mean... your opinion is as good as mine? Maybe would be better to add something that points to the snippet file in the description?

Ideally we'd be able to differentiate between something that exists vs something that does not exist. But there's the theme check for that so 🤷

@jamesmengo
Copy link
Contributor Author

Snippet exists, but no liquidDoc

image

Snippet does not exist

image

  • Renders a red squiggly + warning via theme check

Copy link
Contributor

@karreiro karreiro left a comment

Choose a reason for hiding this comment

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

Thank you for this PR, @jamesmengo.

I don't feel strongly about this, but I believe this is the ideal presentation for an undocumented snippet:
image

As a user, it feels like the language server has identified the snippet successfully, it just doesn't have documentation to present, so it feels like it's working well.

Here, we're just showing a link, and it feels like the language server has not identified that snippet:
image

I personally believe the first one is more satisfying to use, but that's a personal opinion so I'm happy with both directions.

Copy link
Contributor Author

@karreiro I'm on board with going in that direction! I like your line of reasoning

@jamesmengo jamesmengo changed the base branch from jm/param_hover to graphite-base/729 January 27, 2025 17:14
@jamesmengo jamesmengo force-pushed the jm/prevent_hover_nonexistent_snippets branch from 16db3dd to 6414426 Compare January 27, 2025 17:14
@jamesmengo jamesmengo changed the base branch from graphite-base/729 to main January 27, 2025 17:15
@jamesmengo jamesmengo force-pushed the jm/prevent_hover_nonexistent_snippets branch from 6414426 to 9560ce1 Compare January 27, 2025 17:15
@jamesmengo jamesmengo force-pushed the jm/prevent_hover_nonexistent_snippets branch from 9560ce1 to 6f1ddbb Compare January 27, 2025 17:53
Copy link
Contributor Author

jamesmengo commented Jan 27, 2025

Merge activity

  • Jan 27, 12:53 PM EST: A user started a stack merge that includes this pull request via Graphite.
  • Jan 27, 12:53 PM EST: A user merged this pull request with Graphite.

@jamesmengo jamesmengo merged commit f28e72d into main Jan 27, 2025
7 checks passed
@jamesmengo jamesmengo deleted the jm/prevent_hover_nonexistent_snippets branch January 27, 2025 17:53
@jamesmengo jamesmengo added the #gsd:44310 LiquidDoc label Feb 3, 2025 — with Graphite App
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
#gsd:44310 LiquidDoc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants