Skip to content

Conversation

tomthorogood
Copy link
Contributor

@tomthorogood tomthorogood commented May 19, 2023

When reading through the StyledOcticon docs, I had a hard time figuring out where to import the *Icons from, and had to grep the primer/react code base for bit before I found it. I figured this would help others from having to do the same.

Merge checklist

  • [ ] Added/updated tests
  • Added/updated documentation
  • [ ] Changes are SSR compatible
  • [ ] Tested in Chrome
  • [ ] Tested in Firefox
  • [ ] Tested in Safari
  • [ ] Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

@changeset-bot
Copy link

changeset-bot bot commented May 19, 2023

⚠️ No Changeset found

Latest commit: b09fc00

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@tomthorogood tomthorogood temporarily deployed to github-pages May 19, 2023 23:11 — with GitHub Actions Inactive
@tomthorogood tomthorogood marked this pull request as ready for review May 19, 2023 23:26
@tomthorogood tomthorogood requested review from a team and broccolinisoup May 19, 2023 23:26
Copy link
Member

@broccolinisoup broccolinisoup left a comment

Choose a reason for hiding this comment

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

👋🏻 @tomthorogood, sorry to hear about your experience! Great idea to add the import paths, thanks so much for that 🙏🏻

@tomthorogood
Copy link
Contributor Author

@broccolinisoup do you have any advice on how to correct the CI / format check failure? (The changeset failure is not one I can remedy.)

@broccolinisoup
Copy link
Member

👋🏻 @tomthorogood If you could run npm run format:diff in the root folder, hopefully that should fix it. Let me know if you run into any issues.

Re the changeset, I added skip changeset label to the PR and it should be good now. We usually don't publish a changeset for documentation changes. Let me know if you have any concern though!

@broccolinisoup broccolinisoup added the skip changeset This change does not need a changelog label May 22, 2023
@tomthorogood tomthorogood temporarily deployed to github-pages May 23, 2023 19:18 — with GitHub Actions Inactive
@broccolinisoup
Copy link
Member

@tomthorogood I see in the diff there are version changes on the package-lock.json but no package.json included. Was that intentional?

@tomthorogood
Copy link
Contributor Author

@broccolinisoup I assumed that was expected from running the format script; I didn't intend it, though.

@broccolinisoup
Copy link
Member

@tomthorogood Interesting, as far as I know format running shouldn't cause a package-lock changes. Could you try removing it from the diff and make sure only the mdx changes come through? Thank you 🙏🏻

@joshblack joshblack enabled auto-merge June 20, 2023 16:12
@joshblack joshblack temporarily deployed to github-pages June 20, 2023 16:18 — with GitHub Actions Inactive
@joshblack joshblack temporarily deployed to github-pages June 20, 2023 17:14 — with GitHub Actions Inactive
@joshblack joshblack added this pull request to the merge queue Jun 26, 2023
Merged via the queue into primer:main with commit 2ef429c Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip changeset This change does not need a changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants