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

Gatsby v5 : code snippet bugs #1439

Open
2 of 3 tasks
Tracked by #1324
alisonjoseph opened this issue Mar 8, 2024 · 17 comments
Open
2 of 3 tasks
Tracked by #1324

Gatsby v5 : code snippet bugs #1439

alisonjoseph opened this issue Mar 8, 2024 · 17 comments
Assignees
Milestone

Comments

@alisonjoseph
Copy link
Member

alisonjoseph commented Mar 8, 2024

Moving over code snippet specific bugs from #1425 in addition to some new ones.

  • Remove new hideCode prop added here: Fix: code blocks in mdx + issues/868 #1422 Show more button needs to show/hide by default, the Carbon component should do this out of the box if we can use that.
  • Copy icon isn't visible (color/token is wrong)
  • Can we find a way to not break code snippets that don't specify a language
@alisonjoseph
Copy link
Member Author

Hi @muditjuneja thanks for all your help with this so far! Not sure if you have time to take a look at any of the above issues I noticed. Its so close

@muditjuneja
Copy link
Contributor

Hey @alisonjoseph I will look into these soon.

@muditjuneja
Copy link
Contributor

can you clarify the this -
Remove new hideCode prop added here: https://github.com/carbon-design-system/gatsby-theme-carbon/pull/1422 Show more button needs to show/hide by default, the Carbon component should do this out of the box if we can use that.
What else should we cover in this?

@muditjuneja
Copy link
Contributor

Hi @eng618 the styles are not being applied properly on the copy button due to the conflicts in styling. I guess, All those webpack warnings when building and starting the dev loop are playing a role here. Do you know what are all these conflicts?

@eng618
Copy link
Collaborator

eng618 commented Mar 10, 2024

Hi @eng618 the styles are not being applied properly on the copy button due to the conflicts in styling. I guess, All those webpack warnings when building and starting the dev loop are playing a role here. Do you know what are all these conflicts?

No I’m not familiar with that, and the styling isn’t really my strong area. @alisonjoseph is there anyone who can help take a look at this?

@alisonjoseph
Copy link
Member Author

I can take a look and get a fix in for the icon color.

With regards to the new hideCode prop this feels like a regression since currently the component does this automatically. It would be ideal if we didn't have to manually add this prop and the component handled the logic when the show more button displays.

@muditjuneja
Copy link
Contributor

Yeah that makes sense. Actually there is already a logic that if there are 9 lines of code, it would automatically show the button and hide the code that we changed in the next branch. I can change the defaultValue to true which will provide the same behaviour and remove this regression. And if anyone want the other behaviour, they would pass the value as false

@alisonjoseph
Copy link
Member Author

@muditjuneja oh ok, yes I think having it default to true would be great. thank you!

@alisonjoseph
Copy link
Member Author

alisonjoseph commented Mar 13, 2024

@muditjuneja looking at this locally and the hideCode prop doesn't appear to be doing anything at all, curious why we need this at all? What would be the use-case for someone to not want the show more button for longer code blocks?

@muditjuneja
Copy link
Contributor

If there is a code block having more than 9 lines of code then this would come into play.
This would be useful if someone wants to show the entire codeblock to give the entire context without letting user do anything (or click)

@alisonjoseph
Copy link
Member Author

@muditjuneja got it, I just opened a PR with an update to the docs page, I don't believe it is working.

@eng618
Copy link
Collaborator

eng618 commented Mar 15, 2024

This still seems a little clunky and not intuitive... Just thinking this out... what if instead of this boolean logic, we default to always showing the Show More button, but if we add a prop showAll then it removes the show all button and shows the full code block.

Similar to Caption: https://gatsby.carbondesignsystem.com/components/Caption with the fullWidth prop.

@muditjuneja thoughts?

@muditjuneja
Copy link
Contributor

fullWidth is also a bool but I think showAll makes more sense as the prop name. Here is the PR for this change - #1445
Can you review this?

@alisonjoseph
Copy link
Member Author

Testing the release against the Carbon website and looks like bash isn't recognized as a language for some reason.

```bash
Screenshot 2024-03-20 at 8 51 32 AM

@alisonjoseph alisonjoseph added this to the 2024 Q2 milestone Mar 21, 2024
@eng618
Copy link
Collaborator

eng618 commented Mar 21, 2024

Testing the release against the Carbon website and looks like bash isn't recognized as a language for some reason.


```bash

Screenshot 2024-03-20 at 8 51 32 AM

Interesting I've defaulted lately to using sh or shell lately

@eng618
Copy link
Collaborator

eng618 commented Mar 25, 2024

It is a supported language though: https://prismjs.com/#supported-languages

@eng618
Copy link
Collaborator

eng618 commented Mar 25, 2024

Actually, looks like we would have to add that manually... they only include these base languages by default: https://github.com/FormidableLabs/prism-react-renderer/blob/c914fdea48625ba59c8022174bb3df1ed802ce4d/packages/generate-prism-languages/index.ts#L9-L23

  "jsx",
  "tsx",
  "swift",
  "kotlin",
  "objectivec",
  "js-extras",
  "reason",
  "rust",
  "graphql",
  "yaml",
  "go",
  "cpp",
  "markdown",

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ⏱ Backlog
Development

No branches or pull requests

3 participants