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

Restrict tags for HiddenTextExpander and Truncate #581

Merged
merged 3 commits into from
May 18, 2021

Conversation

khiga8
Copy link
Contributor

@khiga8 khiga8 commented May 17, 2021

What
This PR restricts tags for HiddenTextExpander to :span, and also restricts tags for Truncate to [:div, :span, :p]. I chose these tags based on current usage in dotcom and what seemed reasonable.

This PR also makes explicit the tag argument for Markdown so consumers know they can set the tag argument. I was unsure what the allowed tags for this component should be based on current usage of article, tdin dotcom.test_renders_different_tags` also seemed to imply that this component can be relatively flexible so I did not add restrictions. Open to thoughts if you have any.

Why
Part of #491

@khiga8 khiga8 requested a review from a team May 17, 2021 18:19
@vercel
Copy link

vercel bot commented May 17, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/primer/view-components/3Eh2oncxFVRvb9fAudeMUynMnKAG
✅ Preview: https://view-components-git-kh-setdefaulttruncatehiddentext-primer.vercel.app

@khiga8 khiga8 force-pushed the kh-set_default_truncate_hidden_text branch from 8f39182 to 9deec64 Compare May 17, 2021 18:27
@vercel vercel bot temporarily deployed to Preview May 17, 2021 18:27 Inactive
@joelhawksley
Copy link
Contributor

@manuelpuyol you wrote test_renders_different_tags. Did you intend to imply that any tag could be passed for Markdown?

@manuelpuyol
Copy link
Contributor

@manuelpuyol you wrote test_renders_different_tags. Did you intend to imply that any tag could be passed for Markdown?

Looking at dotcom usage for markdown-body we also use it with td and article. The test was intended to guarantee that we could use those tags
I think that restricting that component to :div, :td, :article is fine!

@khiga8 khiga8 force-pushed the kh-set_default_truncate_hidden_text branch from 9deec64 to 9103df7 Compare May 17, 2021 20:21
@vercel vercel bot temporarily deployed to Preview May 17, 2021 20:21 Inactive
@vercel vercel bot temporarily deployed to Preview May 17, 2021 20:23 Inactive
@khiga8 khiga8 force-pushed the kh-set_default_truncate_hidden_text branch from 5b4b3fe to 2160e30 Compare May 17, 2021 20:26
@vercel vercel bot temporarily deployed to Preview May 17, 2021 20:26 Inactive
@khiga8 khiga8 force-pushed the kh-set_default_truncate_hidden_text branch from 2160e30 to 8f3b4a2 Compare May 17, 2021 22:03
@vercel vercel bot temporarily deployed to Preview May 17, 2021 22:03 Inactive
@khiga8 khiga8 force-pushed the kh-set_default_truncate_hidden_text branch from 8f3b4a2 to 12264e0 Compare May 17, 2021 23:23
@vercel vercel bot temporarily deployed to Preview May 17, 2021 23:23 Inactive
@khiga8 khiga8 force-pushed the kh-set_default_truncate_hidden_text branch from 12264e0 to 9718b9e Compare May 17, 2021 23:28
@vercel vercel bot temporarily deployed to Preview May 17, 2021 23:28 Inactive
CHANGELOG.md Outdated
Comment on lines 25 to 28
### Breaking changes

* Restrict allowed tags for `Truncate`, `Markdown`, and `HiddenTextExpander`.

Copy link
Contributor

Choose a reason for hiding this comment

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

mind updating this changelog to be after # main?

@khiga8 khiga8 force-pushed the kh-set_default_truncate_hidden_text branch from 9718b9e to b6f8fc2 Compare May 18, 2021 16:38
@vercel vercel bot temporarily deployed to Preview May 18, 2021 16:38 Inactive
@khiga8 khiga8 enabled auto-merge May 18, 2021 16:42
@vercel vercel bot temporarily deployed to Preview May 18, 2021 17:59 Inactive
@khiga8 khiga8 merged commit f305bc1 into main May 18, 2021
@khiga8 khiga8 deleted the kh-set_default_truncate_hidden_text branch May 18, 2021 20:08
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.

3 participants