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

tpl/tplimpl: Add details shortcode #13100

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

racehd
Copy link

@racehd racehd commented Dec 1, 2024

  • Add new shortcode to render details HTML element.
  • Implement integration tests to check: default state, custom summary, open state, safeHTML sanitization, allowed attributes, and localization of default summary text.
  • Update docs to include details shortcode.

Closes #13090

@CLAassistant
Copy link

CLAassistant commented Dec 1, 2024

CLA assistant check
All committers have signed the CLA.

docs/content/en/content-management/shortcodes.md Outdated Show resolved Hide resolved
docs/content/en/content-management/shortcodes.md Outdated Show resolved Hide resolved
docs/content/en/content-management/shortcodes.md Outdated Show resolved Hide resolved
docs/content/en/content-management/shortcodes.md Outdated Show resolved Hide resolved
docs/content/en/content-management/shortcodes.md Outdated Show resolved Hide resolved
Copy link
Member

@jmooring jmooring left a comment

Choose a reason for hiding this comment

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

The AssertFileContent method iterates over the values and looks for a match. So it tests if the result contains the value, but it does not test if they are in the given order. I would try to combine these into a single string, possibly using AssertFileContentEquals instead.

Also, I think our convention with multiple values is to place the closing parenthesis on its own line, e.g.,

	b.AssertFileContent("foo",
		"bar",
		"baz",
	)

@racehd
Copy link
Author

racehd commented Dec 1, 2024

The AssertFileContent method iterates over the values and looks for a match. So it tests if the result contains the value, but it does not test if they are in the given order. I would try to combine these into a single string, possibly using AssertFileContentEquals instead.

Thank you for the review! I had noticed AssertFileContent did not check the order. I previously had tried AssertFileContentEquals but it will also check for newlines and whitespace which I thought may make it more prone to triggering false positives in the future.

The following would pass the checks:

AssertFileContentEquals
	// Test1: default state (closed by default)
	b.AssertFileContentEquals("public/d1/index.html",
	"\n<details>\n    <summary>Details</summary>\n    <p>Basic example without summary</p>\n</details>\n",
	)
	content1 := b.FileContent("public/d1/index.html")
	c := qt.New(t)
	c.Assert(content1, qt.Not(qt.Contains), "open")

	// Test2: custom summary
	b.AssertFileContentEquals("public/d2/index.html",
	"\n<details>\n    <summary>Custom Summary</summary>\n    <p>Example with custom summary text</p>\n</details>\n",
	)

	// Test3: open state
	b.AssertFileContentEquals("public/d3/index.html",
	"\n<details open>\n    <summary>Test Open State</summary>\n    <p>Example with open state</p>\n</details>\n",
	)

	// Test4: safeHTML sanitization
	b.AssertFileContentEquals("public/d4/index.html",
	"\n<details style=\"color: red;\">\n    <summary>Test Attribute sanitization</summary>\n    <p>Example testing attribute sanitization</p>\n</details>\n",
	)
	content4 := b.FileContent("public/d4/index.html")
	c.Assert(content4, qt.Not(qt.Contains), "onclick")
	c.Assert(content4, qt.Not(qt.Contains), "alert")

	// Test5: class attribute
	b.AssertFileContentEquals("public/d5/index.html",
	"\n<details class=\"custom-class\">\n    <summary>Details</summary>\n    <p>Example with allowed class attribute</p>\n</details>\n",
	)

	// Test6: localization
	b.AssertFileContentEquals("public/es/d6/index.html",
	"\n<details>\n    <summary>Detalles</summary>\n    <p>Localization example without summary</p>\n</details>\n",
	)

Alternatively, I could write a helper function to ignore whitespace and make it a little more robust. Do you have a preference on this?

@jmooring
Copy link
Member

jmooring commented Dec 1, 2024

Do you have a preference on this?

I think what you have is fine.

Copy link
Member

@jmooring jmooring left a comment

Choose a reason for hiding this comment

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

Just three minor changes.

tpl/tplimpl/embedded/templates/shortcodes/details.html Outdated Show resolved Hide resolved
docs/content/en/content-management/shortcodes.md Outdated Show resolved Hide resolved
@jmooring
Copy link
Member

jmooring commented Dec 1, 2024

Also, please squash commits.

@racehd racehd force-pushed the feat/details-shortcode-13090 branch from 8ee5af8 to 52beeac Compare December 1, 2024 17:58
@jmooring
Copy link
Member

jmooring commented Dec 1, 2024

Please clean up the commit message and force push. It should look something like this:

- Add new shortcode to render details HTML element.
- Implement integration tests to check: default state, custom summary, open state, safeHTML sanitization, allowed attributes, and localization of default summary text.
- Update docs to include details shortcode.

Closes # 13090

You don't need all the messages from the squashed commits.

- Add new shortcode to render details HTML element.
- Implement integration tests to check: default state, custom summary, open state, safeHTML sanitization, allowed attributes, and localization of default summary text.
- Update docs to include details shortcode.

Closes gohugoio#13090
@racehd racehd force-pushed the feat/details-shortcode-13090 branch from 52beeac to 0c1ec70 Compare December 1, 2024 18:05
@racehd
Copy link
Author

racehd commented Dec 1, 2024

Got it, thanks again

@jmooring
Copy link
Member

jmooring commented Dec 1, 2024

This looks and works great! I recommend we merge this after the tests go green.

@bep
Copy link
Member

bep commented Dec 3, 2024

Two comments from me:

We cannot have this:

 {{- printf " %s=%q" $k $v | safeHTMLAttr }}

I understand it's ... usefulness, but it would violate Hugo's security policy.

Also:

{{- $summary := or (.Get "summary") (T "details") "Details" }}

I'm not sure if we're using the pattern above elsewhere, but I think we need to need to namespace the translation keys. I'm not sure what, but details seems too generic.

Also, for the future, it would be great if we could wait with the implementation until the issue goes from Proposal to Enhancement. I understand the desire for speed/momentum, but ...

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.

Add Details Shortcode for collapsable sections within markdown content
4 participants