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] Hover: Wrap optional parameter names in [] #735

Merged
merged 2 commits into from
Feb 6, 2025

Conversation

jamesmengo
Copy link
Contributor

@jamesmengo jamesmengo commented Jan 25, 2025

What are you adding in this PR?

Closes https://github.com/Shopify/develop-advanced-edits/issues/525

Render a [] around optional parameters when hovering inside a {% render %} tag

image

Before you deploy

  • I included a minor bump changeset
  • My feature is backward compatible

@jamesmengo jamesmengo changed the title Wrap optional parameters name in [] in hover tooltip ----- Behaviour diverges slightly from JSDoc here. JSDoc does not indicate that it's optional, rather it implicitly adds | undefined to the types [LiquidDoc] Hover: Wrap optional parameter names in [] Jan 25, 2025
@jamesmengo jamesmengo force-pushed the jm/prettier_opt_param branch from 40ad9c4 to 299b434 Compare January 27, 2025 17:15
@jamesmengo jamesmengo force-pushed the jm/prettier_opt_param branch from 299b434 to 35d7e8f Compare January 27, 2025 17:54
@jamesmengo jamesmengo force-pushed the jm/hover_opt_param branch 2 times, most recently from 8d0f019 to 829655f Compare January 27, 2025 18:10
@jamesmengo jamesmengo force-pushed the jm/prettier_opt_param branch from 35d7e8f to 2d086a3 Compare January 27, 2025 20:36
@jamesmengo jamesmengo force-pushed the jm/hover_opt_param branch 3 times, most recently from ca9417b to 5e111e6 Compare January 27, 2025 21:44
@jamesmengo jamesmengo changed the base branch from jm/prettier_opt_param to jm/parse_optional_params January 27, 2025 21:44
@jamesmengo jamesmengo marked this pull request as ready for review January 27, 2025 21:49
@jamesmengo jamesmengo self-assigned this Jan 27, 2025
@jamesmengo jamesmengo added the #gsd:44310 LiquidDoc label Jan 27, 2025
@jamesmengo jamesmengo requested review from karreiro and a team January 27, 2025 21:50
@jamesmengo jamesmengo changed the base branch from jm/parse_optional_params to jm/prettier_opt_param January 27, 2025 22:13
@jamesmengo jamesmengo marked this pull request as draft January 27, 2025 22:20
@jamesmengo jamesmengo force-pushed the jm/prettier_opt_param branch from 2d086a3 to 9acae35 Compare January 28, 2025 01:38
@jamesmengo jamesmengo force-pushed the jm/prettier_opt_param branch from 9acae35 to 3c791ea Compare January 28, 2025 01:46
@jamesmengo jamesmengo force-pushed the jm/prettier_opt_param branch from 3c791ea to 7038766 Compare February 4, 2025 03:16
@jamesmengo jamesmengo force-pushed the jm/prettier_opt_param branch from 7038766 to fa9a764 Compare February 4, 2025 22:11
@jamesmengo jamesmengo force-pushed the jm/prettier_opt_param branch from fa9a764 to dd32d08 Compare February 4, 2025 22:17
@jamesmengo jamesmengo force-pushed the jm/prettier_opt_param branch from dd32d08 to e0d3790 Compare February 4, 2025 22:20
@jamesmengo jamesmengo force-pushed the jm/hover_opt_param branch 3 times, most recently from e285ac4 to 3eede72 Compare February 4, 2025 22:48
@jamesmengo jamesmengo force-pushed the jm/prettier_opt_param branch from e0d3790 to 51fe764 Compare February 4, 2025 22:52
@jamesmengo jamesmengo force-pushed the jm/prettier_opt_param branch 3 times, most recently from c085ac9 to 357feaa Compare February 4, 2025 22:56
);
};

beforeEach(async () => {
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 want to make extending custom cases easier, rather than having a massive test case that's hard to read / understand

@jamesmengo jamesmengo marked this pull request as ready for review February 4, 2025 23:10
@jamesmengo jamesmengo requested review from aswamy and EvilGenius13 and removed request for karreiro February 4, 2025 23:10
Copy link
Contributor

@aswamy aswamy left a comment

Choose a reason for hiding this comment

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

Looks great! One question for discussion, but not required.

private buildParameters(parameters: LiquidDocParameter[]) {
return parameters
.map(({ name, type, description, required }: LiquidDocParameter) => {
const nameStr = required ? `\`${name}\`` : `\`[${name}]\``;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wrote JS doc for a typescript function for optional params. I even had square bracket for optional params, but when you hover over the function, it doesn't show it with square brackets in the hover text.

  • Should we do something like for ours as well?
  • Or should we have an explicit "optional" after the paramName?
image image

Suggestion:
image

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 like this idea! It's more expressive. Let's go in this direction :)

Base automatically changed from jm/prettier_opt_param to main February 6, 2025 01:08
@jamesmengo jamesmengo merged commit 892dcbd into main Feb 6, 2025
7 checks passed
@jamesmengo jamesmengo deleted the jm/hover_opt_param branch February 6, 2025 01:19
@jamesmengo jamesmengo removed the #gsd:44310 LiquidDoc label Feb 6, 2025
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