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

Convert ERB octicon() helpers to inline SVG on the fly #847

Merged
merged 17 commits into from
Aug 7, 2019

Conversation

shawnbot
Copy link
Contributor

Fixes #840?!

@vercel
Copy link

vercel bot commented Jul 22, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

@shawnbot shawnbot changed the base branch from master to release-12.5.0 July 22, 2019 21:58
@colebemis
Copy link
Contributor

@shawnbot This is neat! Would the the code block contain the code before or after this transformation happens? It seems like we'll end up displaying <svg>...</svg> instead of <%= octicon "check" %>

@shawnbot
Copy link
Contributor Author

@shawnbot This is neat! Would the the code block contain the code before or after this transformation happens? It seems like we'll end up displaying <svg>...</svg> instead of <%= octicon "check" %>

@colebemis Right now it adds an comment before the HTML, but making react-live convert it on the fly is much trickier.

@colebemis
Copy link
Contributor

Gotcha. FYI, @broccolini, @emplums and I discussed using octicons-react for the Primer CSS code examples instead of fighting with the erb templates.

Copy link
Contributor

@simurai simurai left a comment

Choose a reason for hiding this comment

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

Hmmm.. it doesn't seem that the examples get rendered:

image

When trying locally, I can see a bunch of

Replacing 3 ERB block(s)...

@vercel vercel bot requested a deployment to staging July 23, 2019 17:42 Abandoned
@vercel vercel bot requested a deployment to staging July 23, 2019 17:43 Abandoned
@vercel vercel bot requested a deployment to staging July 23, 2019 18:13 Abandoned
@shawnbot
Copy link
Contributor Author

shawnbot commented Jul 23, 2019

@shawnbot shawnbot mentioned this pull request Jul 30, 2019
11 tasks
@shawnbot shawnbot mentioned this pull request Aug 7, 2019
@shawnbot shawnbot changed the base branch from release-12.5.0 to release-12.6.0 August 7, 2019 20:47
@vercel vercel bot temporarily deployed to staging August 7, 2019 20:57 Inactive
@@ -14,6 +14,7 @@ import {CONTENT_MAX_WIDTH} from '../docs/constants'
import {repository} from '../package.json'

import '../src/index.scss'
import '@primer/octicons/index.scss'
Copy link
Contributor Author

@shawnbot shawnbot Aug 7, 2019

Choose a reason for hiding this comment

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

We can delete this if/when #857 lands alongside this.

Suggested change
import '@primer/octicons/index.scss'

@shawnbot
Copy link
Contributor Author

shawnbot commented Aug 7, 2019

Okay @simurai, when you're back this is ready to test out and review. 🙏

@shawnbot shawnbot requested a review from jonrohan August 7, 2019 21:39
@shawnbot
Copy link
Contributor Author

shawnbot commented Aug 7, 2019

And @colebemis, circling back on your comments above:

IMO, it'd be great if there could be two "tabs" for each of our ERB examples: one with the original ERB in tact for folks to copy and paste without editing; and the other static HTML, which we'd generate from this.

/cc @feministy who had some good feedback on the comments that this outputs. 😁

Copy link
Member

@jonrohan jonrohan left a comment

Choose a reason for hiding this comment

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

Looks good 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants