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: add minor improvements #604

Conversation

magicmatatjahu
Copy link
Member

@magicmatatjahu magicmatatjahu commented May 31, 2022

Description

Changes proposed in this pull request:

  • Set the left menu collapsable:

    • expanded:

      image

    • collapsed

      image

  • add collapse button for nested subschemas in the Schema component:

    image
    image

  • add operation's summary to the left navigation:

    image

  • add anchors in headers:

    image
    image

Related issue(s)
Resolves #441
Resolves #440
Resolves #599
Resolves #605

Preview: https://3000-magicmatatj-asyncapirea-pl74cof8gqr.ws-eu46.gitpod.io/
Gitpod url (you need to login by github accout): https://gitpod.io/#https://github.com/asyncapi/asyncapi-react/pull/604

cc @mcturco @derberg

@magicmatatjahu magicmatatjahu added the enhancement New feature or request label May 31, 2022
@magicmatatjahu magicmatatjahu changed the title Next branches/minor improvements feat: add minor improvements May 31, 2022
@derberg
Copy link
Member

derberg commented Jun 7, 2022

you do not like to split PR to make it easier for reviewers don't you?

Set the right menu collapsable

isn't it left? maybe there should be a props to control that behaviour, like collapse by default?

add operation's summary to the left navigation

this doesn't look good, could be disabled by default and enabled only be props

Preview: https://3000-magicmatatj-asyncapirea-pl74cof8gqr.ws-eu46.gitpod.io/

doesn't work

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@magicmatatjahu
Copy link
Member Author

@derberg Thanks for review and sorry for such a delay.

you do not like to split PR to make it easier for reviewers don't you?

I wouldn't want to split this into 5 separate PRs. I know about problems for reviewers, but the changes are very small, but due to having to do an update of the styles, html as well as the js itself there is much more changed code than there should be. For example, anchors resulted in update of several components because I had to add new functionality for each.

isn't it left? maybe there should be a props to control that behaviour, like collapse by default?

Yeah... left 🤦🏼‍♂️ I also added prop config.sidebar.collapsed for it and update docs.

this doesn't look good, could be disabled by default and enabled only be props

I don't want to add special prop for something like this and don't know why it looks bad. You have the address of the channel and a short description of it. Probably on live preview it should be more friendly for eyes.

doesn't work

yeah I know, because each preview is created in separate workspace, and after some hours it's deleted. Better will be if you will open https://gitpod.io/#https://github.com/asyncapi/asyncapi-react/pull/604 login by Github, wait for installing deps and open 3000 port.

@derberg
Copy link
Member

derberg commented Jun 28, 2022

I wouldn't want to split this into 5 separate PRs. I know about problems for reviewers, but the changes are very small, but due to having to do an update of the styles, html as well as the js itself there is much more changed code than there should be. For example, anchors resulted in update of several components because I had to add new functionality for each.

yeah, you can definitely get some inspiration from @jonaslagoni and NATS template https://github.com/asyncapi/dotnet-nats-template/pulls?q=is%3Apr+is%3Aclosed. He did a pro work there, separate PR per each feature and each fix

I don't want to add special prop for something like this and don't know why it looks bad

I wrote it doesn't look good, not that it looks bad 😆 it just looks cluttered. Left nav is not very "wide" so descriptions make it look super cluttered. This is why I vote for prop that by default disables it. We should not throw something like this at users

@fmvilas
Copy link
Member

fmvilas commented Jul 5, 2022

@magicmatatjahu I see you're splitting this PR already, which is awesome 🙏 Please don't forget to close this one.

@magicmatatjahu
Copy link
Member Author

magicmatatjahu commented Jul 5, 2022

I'm closing this PR in favor of other, smaller ones.

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

Successfully merging this pull request may close these issues.

3 participants