-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[polaris.shopify.com] Include HTML source for examples #6688
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
Conversation
| height={iframeHeight} | ||
| width="100%" | ||
| onLoad={handleExampleLoad} | ||
| ref={iframeRef} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found the refs approach to be flaky. Maybe because the ref changes due to the Tabs. Using the id to find the iframe consistency works.
| } | ||
| let attempts = 0; | ||
|
|
||
| const waitForExampleContent = setInterval(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just because the iframe's onLoad handler fires doesn't mean that React example has had the time to render. This timer polls the iframe to check if the example has actually been rendered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yayayaya yep yep. iframes are just urgh
chloerice
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great Marten!! 🤌🏽 Had a couple questions and suggestions but nothing blocking 😁
| }[]; | ||
| } | ||
|
|
||
| function Code({ tabs }: Props) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I first read the PR description I assumed that the Code component was a generic wrapper for any code sample on the site, but it looks like it's specific to ComponentExamples. Should we move the sub-component into a components directory inside of the ComponentExamples directory? Can the component be more generic, maybe the tabs prop could be optional? For example, rendering the Code component for the /contributing/shipping-your-contribution code page doesn't completely translate for things like yarn commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is generic! Is there anything about it that made you think otherwise?
The tabs are optional — if you pass in a single example, Code will render it without tabs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just have two components? CodeTabs and CodeBlock or something? We kind of already do with HighlightedCode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I clarified the component props a bit which maybe (?) makes the component clearer:
To render some code:
<Code code={{ title: "My example", code }} />
To render multiple pieces of code:
<Code code={[
{ title: "Styles", code: cssCode },
{ title: "HTML", code: htmlCode },
]} />
In short: It's not about the tabs, it's about what you want to render. The tabs are just a happy side effect.
Happy to refactor this down the road if it causes confusion!
polaris.shopify.com/src/components/ComponentExamples/ComponentExamples.tsx
Outdated
Show resolved
Hide resolved
| <> | ||
| <h2 id="examples">Examples</h2> | ||
|
|
||
| <Tab.Group |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an opportunity to make the button-like tabs their own sub-component? They might be useful to replace the Select in the QuickStartGuides of the /contributing section as well 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maaaaybe! Although I'm not sure we would want this styling for other types of tabs — it's very specific for the Code component. All the logic and a11y features come from HeadlessUI so that part is already reusable for other parts of the site.
Co-authored-by: Chloe Rice <[email protected]>
…Examples.tsx Co-authored-by: Chloe Rice <[email protected]>
| import { useState } from "react"; | ||
|
|
||
| interface Props { | ||
| tabs: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| tabs: { | |
| tabs?: { |
| <div | ||
| className={styles.ActualCode} | ||
| dangerouslySetInnerHTML={{ | ||
| __html: Prism.highlight(code, Prism.languages.javascript, "javasript"), | ||
| }} | ||
| ></div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be pre instead of div
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe? It seems like prism does some stuff...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Fixed!
| const waitForExampleContent = setInterval(() => { | ||
| const exampleIframe = document.getElementById( | ||
| "examples-iframe" | ||
| ) as HTMLIFrameElement; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Urgh. iFrames suck. When can we use the nextjs beta and try out layout lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we maybe add a comment about what this does or why it's needed? I could imagine coming back here in a week and being confused as heck.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the code to be even more explicit about what's going on, hopefully that makes it self documenting.
I'm also keen not to use iframes at all. Idea: Import the Polaris CSS and prefix every selector with a class?
| "business thumbnail", | ||
| "ios", | ||
| "android" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to make a seperate PR for this. It shouldn't be in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest from main will clean this up.
alex-page
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice clean up, thanks @martenbjork
| __html: Prism.highlight( | ||
| code, | ||
| Prism.languages.javascript, | ||
| "javasript" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found a typo!
also with Prism, it’s best practice to set Prism.manual = true somewhere at the top of this file. It shouldn’t make a difference here because you haven’t used the classes on the containing element that prism searches for to replace the content, but, that can get funky. Prism.manual is also just a flag for the internals of prism not to run unless called so it’s a minor optimization but worth it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the typo here b3e14b7
Are you able to write up an issue around what Prism.manual = true would change or enhance with the code highlighting? I am taking a look and am not too sure I understand if it's needed.
🚀 Adds React/HTML switcher to component examples
🚀 Renames
Examplescomponent toComponentExamples.🚀 Renames
CodeExamplecomponent toCode. Adds ability to contain multiple examples, rendered in tabs.🚀 Uses this component to render code everywhere: examples, markdown, icons page.
🚀 Code now respect settings for light and dark mode. Light code in light mode, dark code in dark mode.
🚀 Fixes the broken
[object object]code in AppProvider docs. We were parsing the markdown both ingetInitialPropsand inReactMarkdowncausing HTML, not markdown, to be passed to theMarkdowncomponent. Stringify HTML and you get[object object]...Closes #6603
Closes #6528
Closes #6341
Closes #6531
Closes #6559
react-html-pr.mp4
Tested with VoiceOver:
voiceover.mov