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

feat: add PrismFormatted #303

Merged
merged 5 commits into from
Sep 10, 2024

Conversation

huyenltnguyen
Copy link
Member

@huyenltnguyen huyenltnguyen commented Sep 6, 2024

Checklist:

This PR adds PrismFormatted component to the library.

This component is cloned from the main repo (https://github.com/freeCodeCamp/freeCodeCamp/blob/main/client/src/templates/Challenges/components/prism-formatted.tsx) with some small adjustments.

Screenshots
Light Dark
Default Screenshot 2024-09-09 at 07 12 32 Screenshot 2024-09-09 at 07 12 40
With line numbers Screenshot 2024-09-09 at 07 12 57 Screenshot 2024-09-09 at 07 12 47

@huyenltnguyen huyenltnguyen force-pushed the feat/prism-formatted branch 3 times, most recently from a3a8700 to 108947c Compare September 8, 2024 23:02
plugins: [
["transform-react-remove-prop-types", { removeImport: true }],
[
"prismjs",
Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -18,7 +18,7 @@ export const parameters = {
},
{
name: "dark-palette",
value: "#0a0a23",
value: "#1b1b32",
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'm changing the background color to match the one we use on /learn (gray90 to gray85).

Without this change, we can't see the code block properly.

Comparison
#0a0a23 #1b1b32
Screenshot 2024-09-09 at 05 40 38 Screenshot 2024-09-09 at 05 40 55

Comment on lines +6 to +11
// Stub out CSS as Jest would try to parse the imported stylesheets as JS modules
// and throw an error as it can't transpile the code.
// Ref: https://github.com/jestjs/jest/issues/3094#issuecomment-385164816
moduleNameMapper: {
"\\.css": "<rootDir>/__mocks__/styleMock.ts",
},
Copy link
Member Author

Choose a reason for hiding this comment

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

Jest couldn't run prism-formatted.test.tsx because it was trying (and failed) to parse both prismjs and our CSS files.

Screenshot of the failure Screenshot 2024-09-09 at 05 45 35

@@ -29,7 +29,7 @@
@apply bg-background-tertiary text-foreground-tertiary;
}
:not(pre) > code {
@apply border-1 border-gray-450;
@apply border-1 border-gray-450 px-[4px] py-[1px];
Copy link
Member Author

Choose a reason for hiding this comment

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

Add some padding to inline code.

Site note (mostly for future me): This :not(pre) > code rule is to cover the case where the consumers don't use PrismFormatted to render strings that contain <code> tags.

@@ -40,6 +40,7 @@
h6,
p {
margin-bottom: 12.5px;
color: var(--foreground-primary);
Copy link
Member Author

Choose a reason for hiding this comment

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

Add a default text color to text elements.

Comment on lines +37 to +42
/* Without this, the above selector takes precedence and messes up the answer
padding in night mode */
.dark-palette .video-quiz-option > pre {
padding: 0;
margin: 0;
}
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 think this rule should be removed as the video-quiz-option class is specific to /learn. Though I'll need to integrate PrismFormatted into Quiz/QuizQuestion so the rule could possibly be moved there.

I'm keeping the rule for now to be safe.

Comment on lines +19 to +21
/**
* PrismFormatted is used to render code blocks with syntax highlighting.
*
Copy link
Member Author

Choose a reason for hiding this comment

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

This wall of text is for the component's description, displayed at the top of the Storybook page.

Screenshot Screenshot 2024-09-09 at 06 46 04

text,
useSpan,
noAria,
codeBlockAriaLabel,
Copy link
Member Author

Choose a reason for hiding this comment

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

In /learn, PrismFormatted automatically creates an aria label with i18next: https://github.com/freeCodeCamp/freeCodeCamp/blob/f1931399a1663f134a80744d3bcb6777d261f96c/client/src/templates/Challenges/utils/index.ts#L70-L75.

fcc/ui doesn't have localization so it needs the string to be passed in.

useSpan,
noAria,
codeBlockAriaLabel,
hasLineNumbers,
Copy link
Member Author

Choose a reason for hiding this comment

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

To enable line numbers, we need to pass the line-numbers class to PrismJS (doc).

The PrismFormatted component in /learn also expects the consumers to pass in the class. But I find this a little error-prone (typo happens), so I decided to turn it into a boolean prop.

@@ -0,0 +1,78 @@
@import "../colors.css";

.light-palette pre[class*="language-"]::selection,
Copy link
Member Author

Choose a reason for hiding this comment

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

The main different between this file and the original one is that:

  • The original uses the :not(.dark-palette) selector
  • This file uses the .light-palette selector

I guess we use :not(.dark-palette) in /learn because we need to account for both (.default and .light-palette). I think that was the case in the past, but it's probably standardized now, and we just use .light-palette or .dark-palette (https://github.com/freeCodeCamp/freeCodeCamp/blob/f1931399a1663f134a80744d3bcb6777d261f96c/client/src/components/layouts/default.tsx#L109-L114).


Note: this .light-palette pre[class*="language-"]::selection selector brings a discrepancy.

In /learn, we are still having this selector with .default, which has no effect, and allows PrismJS to apply its color: https://github.com/freeCodeCamp/freeCodeCamp/blob/f1931399a1663f134a80744d3bcb6777d261f96c/client/src/components/layouts/prism.css#L10-L15.

Changing .default to .light-palette allows the override to take effect.

Comparison
.default .light-palette
Screenshot 2024-09-09 at 07 09 27 Screenshot 2024-09-09 at 07 08 35

@huyenltnguyen huyenltnguyen marked this pull request as ready for review September 9, 2024 00:14
@huyenltnguyen huyenltnguyen requested a review from a team as a code owner September 9, 2024 00:14
src/prism-formatted/utils.ts Outdated Show resolved Hide resolved
src/prism-formatted/utils.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@ojeytonwilliams ojeytonwilliams left a comment

Choose a reason for hiding this comment

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

Nice work, @huyenltnguyen. I'm not the best judge of css changes, but from what I can see everything looks great.

@ahmaxed ahmaxed merged commit d13eb3b into freeCodeCamp:main Sep 10, 2024
7 checks passed
@ahmaxed
Copy link
Member

ahmaxed commented Sep 10, 2024

Thanks for the hard work and the comments.

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