gatsby-theme-carbon icon indicating copy to clipboard operation
gatsby-theme-carbon copied to clipboard

Gatsby v5 : code snippet bugs

Open alisonjoseph opened this issue 4 months ago • 17 comments

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

  • [x] 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.
  • [x] 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 avatar Mar 08 '24 15:03 alisonjoseph

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

alisonjoseph avatar Mar 08 '24 15:03 alisonjoseph

Hey @alisonjoseph I will look into these soon.

muditjuneja avatar Mar 08 '24 19:03 muditjuneja

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 avatar Mar 08 '24 20:03 muditjuneja

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?

muditjuneja avatar Mar 09 '24 21:03 muditjuneja

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?

eng618 avatar Mar 10 '24 02:03 eng618

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.

alisonjoseph avatar Mar 11 '24 18:03 alisonjoseph

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

muditjuneja avatar Mar 12 '24 14:03 muditjuneja

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

alisonjoseph avatar Mar 12 '24 16:03 alisonjoseph

@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?

alisonjoseph avatar Mar 13 '24 16:03 alisonjoseph

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)

muditjuneja avatar Mar 13 '24 17:03 muditjuneja

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

alisonjoseph avatar Mar 13 '24 17:03 alisonjoseph

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?

eng618 avatar Mar 15 '24 18:03 eng618

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?

muditjuneja avatar Mar 16 '24 11:03 muditjuneja

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 avatar Mar 20 '24 13:03 alisonjoseph

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 avatar Mar 21 '24 22:03 eng618

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

eng618 avatar Mar 25 '24 18:03 eng618

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

eng618 avatar Mar 25 '24 18:03 eng618