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

feat: asyncapi specification visualization integration #2898

Merged
merged 40 commits into from
Jul 10, 2024

Conversation

AceTheCreator
Copy link
Member

@AceTheCreator AceTheCreator commented Apr 22, 2024

Implementing the visualization component for the spec using Schyma, yeah I changed the name @derberg 😜.

What changed?

  • This feature brings a host of improvements that I'm thrilled about. For starters, you'll notice that the rendering quality is noticeably better, making the visualizations more appealing and more accessible to interpret.

  • I've also worked hard to ensure better compatibility across a wide range of browsers, so no matter what your preferred browsing tool is, you can expect a seamless experience.

  • I've boosted the overall performance, making everything run smoother and faster. This means less waiting for things to load and more time doing what you do best.

I'm confident these enhancements will make understanding the spec with our visualization component more productive and enjoyable.

What next?

I'm focusing on adding support for allof which seems to be missing in the latest release of Schyma, which to me is the only blocker for moving this PR forward

##TL;DR
Currently, the visualization can be found under /spec-json-schema, but it should be rendered on the docs page(cuz it's more of a doc, tbh). wyt @alequetzalli

cc @derberg @fmvilas @alequetzalli @akshatnema

Copy link

netlify bot commented Apr 22, 2024

Deploy Preview for asyncapi-website ready!

Name Link
🔨 Latest commit 7c9a1ae
🔍 Latest deploy log https://app.netlify.com/sites/asyncapi-website/deploys/668d5b6f4cb23e00088a326d
😎 Deploy Preview https://deploy-preview-2898--asyncapi-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@asyncapi-bot
Copy link
Contributor

asyncapi-bot commented Apr 22, 2024

⚡️ Lighthouse report for the changes in this PR:

Category Score
🔴 Performance 33
🟢 Accessibility 98
🟢 Best practices 92
🟢 SEO 100
🟠 PWA 56

Lighthouse ran on https://deploy-preview-2898--asyncapi-website.netlify.app/

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

This is beautiful ❤️ and imho deserves better promotion than just a link in a banner

Imho it is nothing different from docs, just differently rendered. So should be in navigation, as 3.0.0 - Visualizer or 3.0.0 - Explorer or whatever - but just there.

Screenshot 2024-04-23 at 09 41 54

it should even be here imho, with some different background, to promote it a bit more
Screenshot 2024-04-23 at 09 51 35

@@ -1,3 +1,5 @@
<DocsCards />
Copy link
Member

Choose a reason for hiding this comment

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

this file is just a copy, automation push it out from spec repo, so you cannot add manually anything here

Copy link
Member

Choose a reason for hiding this comment

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

👋🏼

import React from 'react'
import Schyma from 'schyma';
import 'schyma/dist/esm/style.css'
import schema from '../../config/3.0.0.json';
Copy link
Member

Choose a reason for hiding this comment

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

good for now but of course a followup will be needed to make sure we have automation in place in spec-json-schema to push out all new updates to schema to the website repo

@quetzalliwrites
Copy link
Member

Hey, @AceTheCreator, woooooooow 🤯 !! Great addition and so educational. I'M IN LOVE AND I'M SWOONING 😻

Yes, this can certainly belong in the docs. I think it makes sense to place it in the Reference section. I'd prob call it something along the lines of "V3 Visualizer."

quetzalliwrites
quetzalliwrites previously approved these changes May 9, 2024
@@ -86,50 +89,76 @@ export default function DocsLayout({ post, navItems = {}, children }: IDocsLayou

const navigation = posts.docsTree;

const sidebar = (
Copy link
Member

Choose a reason for hiding this comment

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

Can we have a separate component for the DocsSidebar then? It will be then better maintained in future UI tests, instead of creating multiple components in same file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice suggestion @akshatnema, I'll do that :)

@derberg
Copy link
Member

derberg commented Jun 10, 2024

From my perspective - all good. So once you separate the component as @akshatnema mentioned, we are ready to go

@AceTheCreator
Copy link
Member Author

@akshatnema any idea why the lighthouse test is failing?

@derberg
Copy link
Member

derberg commented Jun 13, 2024

@AceTheCreator as contributor, next to failing check, don't you see Details link?

here is what logs in details say:

Error: 1 result for https://deploy-preview-2898--asyncapi-website.netlify.app/en
Report: https://storage.googleapis.com/lighthouse-infrastructure.appspot.com/reports/1718220903993-56479.report.html

❌ `categories.best-practices` failure for `minScore` assertion
Expected >= 0.92, but found 0.83

@AceTheCreator
Copy link
Member Author

@AceTheCreator as contributor, next to failing check, don't you see Details link?

here is what logs in details say:

Error: 1 result for https://deploy-preview-2898--asyncapi-website.netlify.app/en
Report: https://storage.googleapis.com/lighthouse-infrastructure.appspot.com/reports/1718220903993-56479.report.html

❌ `categories.best-practices` failure for `minScore` assertion
Expected >= 0.92, but found 0.83

Of course, I looked at the details 😄

I just wasn't sure if the test was failing because of my changes @derberg

@AceTheCreator
Copy link
Member Author

I also ran the lighthouse test manually with the deploy URL myself, and everything looks good, hence why I wasn't sure why it's failing 🤷🏾‍♂️

@akshatnema
Copy link
Member

@AceTheCreator The score is incredibly low compared to the threshold we have set. Can you please check it on the production environment? There should be some issue with the best practices in the code.

@AceTheCreator
Copy link
Member Author

Thanks for the fix @akshatnema 🫶🏽

@derberg
Copy link
Member

derberg commented Jun 14, 2024

@akshatnema I guess you can approve and merge

Copy link
Member

Choose a reason for hiding this comment

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

@AceTheCreator are you generating this file from a script or it's a json schema for some validations?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's the v3 of the spec-json-schema. The plan is to work on a follow-up pr to make sure we have automation in place in spec-json-schema to push out all new updates to schema to the website repo

Comment on lines +351 to +353
.explorer-menu-wrapper > div > div > div > button {
margin-top: 0px;
}
Copy link
Member

Choose a reason for hiding this comment

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

Can't we add a className or span tag there instead of targeting so many nested div elements?

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason I used nested div was that I want to tailor the change only to affect the menu in the Explorer view

@@ -0,0 +1 @@
<Visualizer />
Copy link
Member

Choose a reason for hiding this comment

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

@AceTheCreator Can you please add metadata for this page, as it will be used as title and description in the head tag?

@akshatnema akshatnema merged commit 85a42be into asyncapi:master Jul 10, 2024
14 checks passed
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.

5 participants