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

Lucia/update nerdpack detail docs #1748

Merged
merged 28 commits into from
Nov 16, 2021
Merged

Conversation

brammerl
Copy link
Contributor

@brammerl brammerl commented Oct 4, 2021

Description

Updates guide images of the new nerdpack detail UI for the following articles:

📢 NOTE 📢
Please do not merge before we turn on the DevEx/nerdpackItem_previews (link) feature flag. That flag controls the UI displaying the images in the "What's inside" tab and we plan to enable it around the end of October once this ticket is complete: [3rd party Nerdpacks] Add artifact level screenshots

Reviewer Notes

If there are links or steps needed to test this work, add them here.

Related Issue(s) / Ticket(s)

Screenshot(s)

If relevant, add screenshots here.

Use Conventional Commits

Please help the maintainers by leveraging the following conventional commit
standards in your pull request title and commit messages.

Use chore

  • for minor changes / additions / corrections to content.
  • for minor changes / additions / corrections to images.
  • for minor non-functional changes / additions to github actions, github templates, package or config updates, etc
git commit -m "chore: adjusting config and content"

Use fix

  • for minor functional corrections to code.
git commit -m "fix: typo and prop error in the code of conduct"

Use feat

  • for major functional changes or additions to code.
git commit -m "feat(media): creating a video landing page"

@gatsby-cloud
Copy link

gatsby-cloud bot commented Oct 4, 2021

Gatsby Cloud Build Report

developer-website-develop

🎉 Your build was successful! See the Deploy preview here.

Build Details

View the build logs here.

🕐 Build time: 1m

Performance

Lighthouse report

Metric Score
Performance 🔶 29
Accessibility 💚 90
Best Practices 🔶 87
SEO 🔶 76

🔗 View full report

@tyreer
Copy link
Contributor

tyreer commented Oct 6, 2021

Thanks for getting this going! 🙌

I think the work in build-apps/serve-publish-subscribe/catalog.mdx is exactly the kind of changes we're looking to introduce. But I wonder if there might be a few other guides that would benefit from updates as part of this work.

Were you able to check the 4 guides we updated in this PR to see if any would benefit from any additional text that would highlight how Nerdpack item uploads are rendered to the What's inside tab?

I know @MichelLosier also collected a bunch of guides here that are worth double checking as well: https://newrelic.atlassian.net/browse/DEVEX-2036?focusedCommentId=974573

@brammerl
Copy link
Contributor Author

brammerl commented Oct 6, 2021

@tyreer and @alexronquillo: implemented the feedback that was noted and also added some new stuff to the create-hello-world-app page.

With regards to ab-testing: it would be a good idea to update the publish page. In order to get screenshots, I would have to go through the whole process. Happy to do so but just wondering if there is anyone who already did that for the already existing screenshots. If so, they could send the file over to me and I could implement the what's inside view to get screenshots.

This publish your nerdpack page also seems like a good target. I have yet to edit it.

Let me know your thoughts.

@@ -246,6 +246,10 @@ nr1 catalog:submit

Return to Instant Observability and refresh the page to see your new screenshots and metadata describing your project.

![Overview View](../../images/create-hello-world/hello-world-overview.png)

![What's Inside View](../../images/create-hello-world/hello-world-whats-inside.png)
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that this seems like a good guide place to mention Nerdpack-item level screenshots

2 suggestions:

  1. Could you add copy to step 3 that describes include Nerdpack-item screenshots? As is, I don't think there's any guidance that the reader should add any images to these directories, so I think the What's inside tab would actually be empty if they clicked it

How we change it to something like:

STEP 3 OF 5
In the root catalog directory of your project, add screenshots or various types of metadata to describe your project. You can also add screenshots in the catalog directories of your launcher or Nerdlet. For details about what you can add, see Update your Nerdpack's catalog metadata

Note that the above suggestion also updates the title and link at the end of step 3, since it seemed to be outdated.

  1. This bottom screen shot, pointing to the What's inside tab, has Apps as its root breadcrumb. For consistency here, and especially since step 5 indicates explicitly that we're coming from I/O, could you replace this screenshot with one where Instant Observability (I/O) is the root breadcrumb?

Copy link
Contributor

Choose a reason for hiding this comment

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

Typically, when there are two screenshots in a row, I like to add some text between them for a transition and for visual relief. Could we maybe add:

And if you added screenshots to your launcher or Nerdlet, you can see them under What's inside:

Copy link
Contributor

Choose a reason for hiding this comment

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

@brammerl would you mind addressing my comment here, as well? I think the second screenshot here could really benefit from its own description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tyreer
Copy link
Contributor

tyreer commented Oct 7, 2021

Do we need all the screenshots still? Seeing the 6 added files on this PR, but only 3 new image additions in the .mdx files.

@tyreer tyreer mentioned this pull request Oct 7, 2021
@tyreer
Copy link
Contributor

tyreer commented Oct 7, 2021

The 2 docs you've updated on this PR feel like great candidates for the change and I think we're pretty close to having those ready to go:

For the A/B testing or the publish page, think I disagree on the need to update those, tbh. The Nerdpack-item uploads don't seem essential to either of those guides.

With regards to ab-testing: it would be a good idea to update the publish page.

This publish your nerdpack page also seems like a good target. I have yet to edit it.

I could see a case made for the A/B test docs. Perhaps that would be a helpful guide to include Nerdpack-item level screenshots. On the other hand, perhaps it would distract from the minimal example of that A/B test app guide.

If we do add to the A/B test guides, then we'd need to be sure to update this step here in Describe your app for the catalog. We'd probably need to include the files a user should upload like that guide does, and include both a launcher and a Nerdlet file.

STEP 5 OF 6
Save the this screenshot to your catalog/screenshots directory.
Users will be able to see a carousel of screenshots in your app's About page in the catalog.

I don't know if those kinds of additional details in these A/B test steps would be a distraction or a helpful illustration. @alexronquillo Curious on your POV in particular on the A/B test docs.

Note: I'm seeing that "the this" type in step 5 above and have a PR here to fix: #1759

@brammerl
Copy link
Contributor Author

brammerl commented Oct 8, 2021

Do we need all the screenshots still? Seeing the 6 added files on this PR, but only 3 new image additions in the .mdx files.

No! I'll make sure to delete the ones that aren't used.

@brammerl
Copy link
Contributor Author

brammerl commented Oct 8, 2021

@tyreer finished up those last edits you suggested. Take your time in looking at them.

tyreer
tyreer previously approved these changes Oct 12, 2021
Copy link
Contributor

@tyreer tyreer left a comment

Choose a reason for hiding this comment

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

One nit suggestions there with the : but otherwise this LGTM! Thanks for all your work and comms on this

@brammerl
Copy link
Contributor Author

@alexronquillo: here are the final changes to the docs :) let us know if you have anything else to add.

@alexronquillo
Copy link
Contributor

I'm not sure what happened. I tried to comment on the A/B test situation last week, but maybe I didn't submit it. Either way, I can't find it here, so here are my thoughts:

The A/B test course works hand in hand with a programmability certification course at learn.newrelic.com. That course is an exhaustive course, covering all things programmability. Because of that, I think we do need to update it.

Unfortunately, I don't have app code to spin one up, but could you create a dummy app with the same name and info, deploy it to the catalog, and use the existing screenshot that's in the course as the nerdlet's screenshot?

@brammerl
Copy link
Contributor Author

@alexronquillo Definitely able to do that. I'll let you know when I've done that and uploaded the corresponding screenshots.

@brammerl
Copy link
Contributor Author

@alexronquillo Finished up adding the screenshot and adding text between the images.

@alexronquillo
Copy link
Contributor

This looks good to me! Ready to merge whenever it's appropriate 🚀

chore: small fixtures
@mehreentahir16 mehreentahir16 force-pushed the lucia/update-nerdpack-detail-docs branch from ad458a6 to e116bee Compare October 27, 2021 20:51
@jpvajda
Copy link
Contributor

jpvajda commented Nov 16, 2021

Is this still a valid PR @brammerl ?

@jpvajda jpvajda added stale PR or issue has been open for a period of time with no activity waiting on contributor labels Nov 16, 2021
@stale stale bot removed the stale PR or issue has been open for a period of time with no activity label Nov 16, 2021
@brammerl
Copy link
Contributor Author

brammerl commented Nov 16, 2021

Is this still a valid PR @brammerl ?

@jpvajda yes it is! I think it needs to be merged in but there are snyk issues that i'm not sure how to resolve. Although after making it ready to review, i see that there are some merge conflicts that I need to take a look at.

EDIT: i resolved the merge conflicts and am letting all of the checks run. Am I good to merge them once they pass? I got the 'green light' to merge a while ago but for some reason never did it.

@brammerl brammerl marked this pull request as ready for review November 16, 2021 22:39
@tyreer
Copy link
Contributor

tyreer commented Nov 16, 2021

This is 🟢 to me! :D

@brammerl brammerl merged commit 6a25339 into develop Nov 16, 2021
@brammerl brammerl deleted the lucia/update-nerdpack-detail-docs branch November 16, 2021 22:53
@nr-opensource-bot
Copy link
Contributor

🎉 This PR is included in version 1.69.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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.

6 participants