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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix screenshot in graphql addon README.md #27111

Closed
wants to merge 1 commit into from

Conversation

RJPercival
Copy link

What I did

Fixes the URL of the screenshot in the GraphQL addon README.md. This does not exist in either main or next, but does exist in master, which is published at:

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

View the preview of the README.md file in GitHub and observe that the screenshot is correctly rendered, rather than missing.

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

馃 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

It was trying to find the screenshot at HEAD, rather than looking at the
`master` branch. The screenshot does not exist at HEAD.

Both of the following pages, which render README.md, refer to the `master`
branch, so this seems like the correct branch to use:

- https://storybook.js.org/addons/@storybook/addon-graphql
- https://www.npmjs.com/package/@storybook/addon-graphql
@RJPercival
Copy link
Author

Looks like CI doesn't work against master any more, unsurprisingly... perhaps the solution here is for the Storybook addon page and npm to be updated to point at https://github.com/storybookjs/addon-graphql?

@jonniebigodes jonniebigodes self-assigned this May 14, 2024
@jonniebigodes
Copy link
Contributor

@RJPercival, thanks for taking the time to put together this pull request and help us improve the documentation by catching this minor issue. However, this pull request is going to be closed. For context, this addon was removed from the monorepo a while ago and enabled here. If you're okay with it, can you make a pull request to update the image in that repository so that it can be picked up by our Integration catalog and potentially by npm?

You can ping me and I'll be more than glad to review it and follow up with you on it.

Let me know and we'll go from there.

Have a great day.

Stay safe

@RJPercival
Copy link
Author

@RJPercival, thanks for taking the time to put together this pull request and help us improve the documentation by catching this minor issue. However, this pull request is going to be closed. For context, this addon was removed from the monorepo a while ago and enabled here. If you're okay with it, can you make a pull request to update the image in that repository so that it can be picked up by our Integration catalog and potentially by npm?

You can ping me and I'll be more than glad to review it and follow up with you on it.

Let me know and we'll go from there.

Have a great day.

Stay safe

Hi @jonniebigodes,

It appears that the linked repository was never actually fully migrated to. Both the integration catalog and npm point to this monorepo. If you let me know once those have been updated to point at that repo, then I'd be happy to create a new PR for that repo instead. Sound ok?

Thanks,
Rob

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

Successfully merging this pull request may close these issues.

None yet

2 participants