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 CodeBox component and code tabs plugin #6038

Merged
merged 12 commits into from
Jan 4, 2024

Conversation

devjvao
Copy link
Contributor

@devjvao devjvao commented Oct 21, 2023

Description

This PR introduces two new components related to the code block:

  • CodeBox: The main component for rendering the code block;
  • CodeTabs: The component that customizes the Tabs component to match the CodeBox style.

And introduces a plugin for resolving the desired format in markdown files into the code tabs structure:

  • codeTabsResolver: The plugin transforms the format below into the code tabs structure.

    ```mjs|cjs labels="MJS,CJS" link="[More options](https://github.com/nodejs/nodejs.org)"
    const { createHmac } = await import('node:crypto');
    --------------
    const { createHmac } = require('node:crypto');
    ``` <!-- prevent markdown from formatting this, ignore it -->

Validation

Single file

image

Multiple files

image

Multiple files with link

image

Considerations

Some things were not clear to me about how the implementation should go.

  • Is there a minimum/maximum width?;
  • Where should the copied notification appear? (top left, top right...);
  • Is it okay to create the CodeTabs for dealing with the tabs structure? I couldn't manage to make it work with a single component;
  • I also had to update the Tabs implementation to accept an addons prop so that we can render it correctly without harming the responsive layout;
  • In the CodeBox component I'm using a languageMap variable for mapping the display language with the language that is used in the markdown. Where should this go? Should this have internationalization support?;
  • In the codeTabsResolver I'm using the normalizeLanguage function to handle JavaScript file extensions like mjs. Is it ok? It seemed fragile to me, but I couldn't think of anything else;
  • The ArrowUpRightIcon looks different from the one used in Figma;
  • The green used in the Tabs component is different from the one used in Figma, so I had to overwrite it, but it seems wrong to do it. Should we update the Tabs green color or remove the style overriding it in the CodeTabs?

Related Issues

Addresses #5819.

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run npx turbo lint to ensure the code follows the style guide. And run npx turbo lint:fix to fix the style errors if necessary.
  • I have run npx turbo format to ensure the code follows the style guide.
  • I have run npx turbo test to check if all tests are passing.
  • I've covered new added functionality with unit tests if necessary.

@devjvao devjvao requested review from a team as code owners October 21, 2023 04:21
@vercel
Copy link

vercel bot commented Oct 21, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 4, 2024 11:00am

Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

Just wanted to say that I truly appreciate the effort you're putting in here, because this component is definitely a truly complex case.

Awesome work so far.

@ovflowd ovflowd added the hacktoberfest This Issue is meant for Hacktoberfest 2023 label Oct 22, 2023
Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

@canerakdas your commit broke a few things:

  • The code lines are not aligned anymore with the line numbers of the CodeBox
  • There's a horizontal scrollbar on CodeBoxes... The line break was intentional as it's cleaner... Could you add it back? Thanks!

@canerakdas
Copy link
Member

  • The code lines are not aligned anymore with the line numbers of the CodeBox

It looks aligned but I added back the leadings 🤔 (Since the font family has been changed, it looks a little confusing in the Chromatic)

  • There's a horizontal scrollbar on CodeBoxes... The line break was intentional as it's cleaner... Could you add it back?

I have reverted this code. Actually, the reason why I added this is that I think it makes it difficult to read the code since the code is not prettified in any way in mobile resolutions;

image

Copy link
Member

@mikeesto mikeesto left a comment

Choose a reason for hiding this comment

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

I agree with @canerakdas , the CodeTabs story looks awkward on a mobile resolution. If the code was formatted with prettier, we could set the printWidth to something that would be a better fit for a smaller screen?

@ovflowd
Copy link
Member

ovflowd commented Jan 4, 2024

Im not sure applying prettier on the code snippets during runtime would be a good idea. Prettier is extremely costly.

We might be able to fix tabulation for mobile with pure CSS. But also, to be honest, this website is not something that I see people accessing from mobile. Node.js website is mostly accessed for downloading Node.js; and sure we have blog and learn content but I'd discourage people actually reading these from mobile anyways

@mikeesto
Copy link
Member

mikeesto commented Jan 4, 2024

For me it's not a blocker to this PR, I'm in favour of seeing it land and overall it looks good. We can always tweak the styling in the future. I agree that many people probably visit the site on a desktop to download Node, but I'm sure we have some users on mobile too

@ovflowd ovflowd merged commit aa0d9d6 into nodejs:main Jan 4, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest This Issue is meant for Hacktoberfest 2023
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create CodeBox component
6 participants