carbon-design-system / gatsby-theme-carbon

A Carbon inspired Gatsby theme
https://gatsby.carbondesignsystem.com/
Apache License 2.0
359 stars 274 forks source link

Gatsby v5 : code snippet bugs #1439

Closed alisonjoseph closed 5 months ago

alisonjoseph commented 8 months ago

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

alisonjoseph commented 8 months ago

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 commented 8 months ago

Hey @alisonjoseph I will look into these soon.

muditjuneja commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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

alisonjoseph commented 8 months ago

@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 commented 8 months ago

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 commented 8 months ago

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

eng618 commented 8 months ago

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 commented 8 months ago

fullWidth is also a bool but I think showAll makes more sense as the prop name. Here is the PR for this change - https://github.com/carbon-design-system/gatsby-theme-carbon/pull/1445 Can you review this?

alisonjoseph commented 8 months ago

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
eng618 commented 8 months ago

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 commented 8 months ago

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

eng618 commented 8 months ago

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",