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

Match punctuation for code card descriptions #10221

Merged
merged 2 commits into from
Oct 12, 2024
Merged

Match punctuation for code card descriptions #10221

merged 2 commits into from
Oct 12, 2024

Conversation

kimprice
Copy link
Member

@kimprice kimprice commented Oct 8, 2024

@kimprice kimprice requested a review from a team October 8, 2024 21:38
@@ -100,7 +100,13 @@ export function renderCodeCard(card: pxt.CodeCard, options: CodeCardRenderOption
}
if (card.description) {
const descr = div(ct, 'ui description');
const shortenedDescription = card.description.split('.')[0] + '.';
const regex = /((?:\.{1,3})|[\!\?…])/;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a nice improvement, also somewhat English-centric. Are there additional characters we should consider here to support all languages?

Copy link
Contributor

Choose a reason for hiding this comment

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

(totally fine to defer additional character support if it isn't straight forward)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll look at the languages we support in the editor and see what else needs to be added.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea I wasn't able to find a collection of end-of-sentence punctuation for all languages. This would require going through each language one by one. I'm going to defer this work for now.

Copy link
Member

Choose a reason for hiding this comment

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

if we are concerned about this, we could always only enable this behavior for languages we know it works for and then just use css text-overflow for the rest

Copy link
Member Author

Choose a reason for hiding this comment

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

@riknoll How would I do that?

Copy link
Contributor

@eanders-ms eanders-ms left a comment

Choose a reason for hiding this comment

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

This is a good improvement and fixes the most glaring cases. Multi-language concerns can we addressed separately, if they're an actual concern.

@kimprice kimprice merged commit f2c9428 into master Oct 12, 2024
6 checks passed
@kimprice kimprice deleted the kim-codeCard branch October 12, 2024 01:38
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.

4 participants