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: manual tools added to the Tools Dashboard #1191

Merged
merged 26 commits into from Jan 18, 2023

Conversation

akshatnema
Copy link
Member

Description
This is a follow-up PR for PR #940 for the addition of tools manually to the Tools Dashboard. This will contain all the changes related to the migration of /docs/tools page to the Tools Dashboard via manual-tools.json file.

Signed-off-by: akshatnema <20bcs022@iiitdmj.ac.in>
Signed-off-by: akshatnema <20bcs022@iiitdmj.ac.in>
@netlify
Copy link

netlify bot commented Dec 25, 2022

Deploy Preview for asyncapi-website ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit eff39d6
🔍 Latest deploy log https://app.netlify.com/sites/asyncapi-website/deploys/63c7ecec71fd7b0008b29f1c
😎 Deploy Preview https://deploy-preview-1191--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 settings.

@github-actions
Copy link

github-actions bot commented Dec 25, 2022

⚡️ Lighthouse report for the changes in this PR:

Category Score
🔴 Performance 46
🟠 Accessibility 88
🟢 Best practices 100
🟢 SEO 100
🔴 PWA 30

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

Signed-off-by: akshatnema <20bcs022@iiitdmj.ac.in>
Copy link
Member Author

@akshatnema akshatnema left a comment

Choose a reason for hiding this comment

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

@derberg There are various issues to be discussed related to the schema. Some of them are present in the comments below, here is one important one.

We can't provide enum to the fields

When we provide enum to a specific field in schema, it restricts the user to use only those specific values that are present in enum variable as array. This, in my opinion, is wrong according to the functionality available with us in the backend. Even if we get values in the tool file which are not predefined in our list, still we have ways to show the Tool in the Dashboard. Using the variable enum, we are restricting users to use those specific values only and not their defined/customizable ones. Here's a preview of what error we are facing right now via enum variable:

Screenshot_20221224_141729

The values in the field are not only judged via spelling, but also on the case in which the value is written. Even Javascript has been rejected due to the enum specified as JavaScript which is wrong according to our defined approach. We have already defined Fuse.js to detect these changes in the spelling and case of the values given by user.

scripts/tools/tools-schema.json Outdated Show resolved Hide resolved
scripts/tools/tools-schema.json Show resolved Hide resolved
scripts/tools/tools-schema.json Show resolved Hide resolved
akshatnema and others added 4 commits December 26, 2022 20:32
Signed-off-by: akshatnema <20bcs022@iiitdmj.ac.in>
Signed-off-by: akshatnema <20bcs022@iiitdmj.ac.in>
Signed-off-by: akshatnema <20bcs022@iiitdmj.ac.in>
@magicmatatjahu
Copy link
Member

@akshatnema Please also fix the example for categories

akshatnema and others added 2 commits January 9, 2023 19:19
Signed-off-by: akshatnema <20bcs022@iiitdmj.ac.in>
@magicmatatjahu
Copy link
Member

I cannot "unclick" the buttons Open Source and Commercial - I need to click Clear filters and that also remove all other filters. We should fix it.

"title": "AsyncAPI-format",
"description": "Format an AsyncAPI document by ordering, casing, formatting, and filtering fields.",
"links": {
"repoUrl": "https://github.com/asyncapi/converter-go"
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it is correct link
should be https://github.com/thim81/asyncapi-format

@derberg
Copy link
Member

derberg commented Jan 13, 2023

@akshatnema enum is not a big problem, solution is anyOf

modify language property to:

              "language": {
                  "type": "string",
                  "description": "Provide information on how other can integrate with the tool in case it is possible. If your code generator generates Python code, then specify Python, even if code generator itself is written in JavaScript. If we forgot about some language then please add it through a pull request to AsyncAPI website repository.",
                  "anyOf" : [
                    {
                      "type": "string",
                      "enum": [
                        "Go",
                        "Java",
                        "JavaScript",
                        "HTML",
                        "C/C++",
                        "C#",
                        "Python",
                        "TypeScript",
                        "Kotlin",
                        "Scala",
                        "Markdown",
                        "YAML",
                        "R",
                        "Rubby",
                        "Rust",
                        "Shell",
                        "Groovy"
                      ]
                    },
                    {
                      "type": "string"
                    }
                  ]
              }

This way, valid will be both:

{
  "title": "AsyncAPI Diff",
  "filters": {
    "language": "Dupa",
    "technology": [
      "TypeScript"
    ],
    "categories": [
      "compare-tool"
    ],
    "hasCommercial": false
  }
}

and

{
  "title": "AsyncAPI Diff",
  "filters": {
    "language": "JavaScript",
    "technology": [
      "TypeScript"
    ],
    "categories": [
      "compare-tool"
    ],
    "hasCommercial": false
  }
}

but autocompletion should still work.
You have to do the same with technology

@magicmatatjahu any objections mister?

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.

all manual tools reviewed

config/tools-manual.json Outdated Show resolved Hide resolved
},
"filters": {
"language": "Java",
"technology": ["Kotlin", "Maven"],
Copy link
Member

Choose a reason for hiding this comment

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

don't want to be a party pooper, but Kotlin is a programming language and looks like we have a case where we can have 2 languages. Like in general in case of JS/TS.

technically it should not be that hard to support. We can support languages as both, string and array, so there is no need to update existing .asyncapi-tool files 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

So should I change the type of language to both string and array of string in schema? It can take some major changes because in this case, we need to change both backend functions and frontend implementation, including Filters and Contexts.

I added Kotlin as same logic you provided for TypeScript once, so I thought that Kotlin can be added in the same way. Also, you can add any dynamic/custom technology you want, there is no issue regarding the matching of technologies with a defined list of technologies.

Copy link
Member

Choose a reason for hiding this comment

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

I think it can be easily handled as a follow-up task. I'm also not 100% sure. I'm always confused what people want in case of JS.

  • If tool is written in JS, it can be used in any library in JS or TS
  • If tool is written in TS, it can be used in any library in JS or TS

I have no idea if developers simply filter by default both JS and TS or not 😄

So yeah, not critical, can be tackled in a separate issue

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, let's create an issue regarding this and have a chat with other contributors too to have an opinion on whether a language should be a string or an array of strings. Many of them will have different notions and ideologies. Let's have a discussion regarding this on a issue 😅

config/tools-manual.json Outdated Show resolved Hide resolved
config/tools-manual.json Outdated Show resolved Hide resolved
config/tools-manual.json Outdated Show resolved Hide resolved
config/tools-manual.json Show resolved Hide resolved
components/tools/ToolsCard.js Outdated Show resolved Hide resolved
config/tools-manual.json Show resolved Hide resolved
config/tools-manual.json Outdated Show resolved Hide resolved
config/tools-manual.json Show resolved Hide resolved
akshatnema and others added 2 commits January 16, 2023 18:34
Co-authored-by: Lukasz Gornicki <lpgornicki@gmail.com>
Signed-off-by: akshatnema <20bcs022@iiitdmj.ac.in>
@akshatnema
Copy link
Member Author

@derberg changes done according to the review. I've left some comments on the review for the discussion.

@derberg
Copy link
Member

derberg commented Jan 16, 2023

@magicmatatjahu anything from your side?

Signed-off-by: akshatnema <20bcs022@iiitdmj.ac.in>
@magicmatatjahu
Copy link
Member

Last thing, can we hardcode width of Open source label? Look below:

image

@akshatnema
Copy link
Member Author

Last thing, can we hardcode width of Open source label? Look below:

Yes, we can. What if I do min-w-[5.3rem] as hardcoded because I think specifying a new spacing parameter for width in tailwind.config.js is not a good call as we are only using this value here. WDYT?

@akshatnema
Copy link
Member Author

@derberg @magicmatatjahu All stuff done 😅. Previous tools which were removed, are added again. Open Source Label is given a hardcoded width and it's looking fine on both mobile and large screen view.

Signed-off-by: akshatnema <20bcs022@iiitdmj.ac.in>
magicmatatjahu
magicmatatjahu previously approved these changes Jan 16, 2023
Signed-off-by: akshatnema <20bcs022@iiitdmj.ac.in>
@akshatnema
Copy link
Member Author

@magicmatatjahu can you please again approve my PR as I mistakenly pushed my Github Token? 😅

@magicmatatjahu
Copy link
Member

@akshatnema You should remove that token from your account :)

@derberg
Copy link
Member

derberg commented Jan 17, 2023

@akshatnema one one thing missing, imho you should remove this section entirely so there is no duplication of content

@akshatnema
Copy link
Member Author

@akshatnema one one thing missing, imho you should remove this section entirely so there is no duplication of content

Tbh, I want to take the opinion of @alequetzalli first, whether we should remove this section from docs or not? because I think this part right now belongs to the documentation and we are still in the phase of the first release of our webpage. We shouldn't go with removing the page used by viewers at one go. Instead, I will prefer to first publicize and promote the Tools Dashboard so that users will aware of them. With some more improvements in the Dashboard, we can soon remove the docs tool page.

@alequetzalli
Copy link
Member

Tbh, I want to take the opinion of @alequetzalli first, whether we should remove this section from docs or not? because I think this part right now belongs to the documentation and we are still in the phase of the first release of our webpage. We shouldn't go with removing the page used by viewers at one go. Instead, I will prefer to first publicize and promote the Tools Dashboard so that users will aware of them. With some more improvements in the Dashboard, we can soon remove the docs tool page.

Oh interesting! ok...

First, I agree with @akshatnema that removing documentation is not related to building this Tool Dashboard. (IOW, it is not duplicate content.) Second, I also agree that we have yet to publicize and promote the Tools Dashboard, so this is not the time to try and delete the tools list/info from Docs. We would need to plan how and why it may make sense to remove that tool list section from docs. Third, I think we should open a Docs GH issue once this dashboard is LIVE that adds a mention of the new dashboard from our Docs. I would add it to the Tools /Overview page.

@derberg
Copy link
Member

derberg commented Jan 18, 2023

https://github.com/asyncapi/website/blob/master/pages/docs/tools/index.md#asyncapi-tools-list is a 1:1 duplication of what we will have in https://asyncapi.com/tools after we merge this PR.

We should remove this duplication as according to https://github.com/asyncapi/community/blob/master/new-tool-documentation.md, community members will no longer add/remove stuff in https://github.com/asyncapi/website/blob/master/pages/docs/tools/index.md#asyncapi-tools-list

Yes, "just remove" is not good if people are used to the old view. We should put something in exchange and promote it more. @akshatnema please open a separate issue for this. I'm ok to merge this without cleanup of https://github.com/asyncapi/website/blob/master/pages/docs/tools/index.md#asyncapi-tools-list, but we need a follow-up issue in case people are confused, so we can share link.

@akshatnema
Copy link
Member Author

/rtm

@asyncapi-bot asyncapi-bot merged commit 7588b7a into asyncapi:master Jan 18, 2023
@akshatnema akshatnema deleted the manual-tools branch January 29, 2023 12:38
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.

None yet

5 participants