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: render bindings in relevant sections #259

Merged
merged 16 commits into from
Mar 23, 2021

Conversation

195858
Copy link
Contributor

@195858 195858 commented Mar 8, 2021

Description

Changes proposed in this pull request:

  • Created a new bindings/binding component, which can render simple, nested and Schema Object properties
  • Added helper functions to determine the type of a binding property, Schema Objects are listed for now as there don't seem to be schemas for bindings at this stage?
  • Added styles for the new component
  • Updated the types to include bindings in the appropriate interfaces
  • Updated server, channel, operation and message components to render bindings if present
  • Slightly changed operation/messages rendering

Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

Choose a reason for hiding this comment

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

Great! Thanks for contribution! I will also make review for this part with rendering in new way the messages/payloads section. Probably we should introduce these changes in followup issue/PR.

library/src/types.ts Show resolved Hide resolved
library/src/helpers/bindingsHelpers.ts Outdated Show resolved Hide resolved
library/src/helpers/bindingsHelpers.ts Show resolved Hide resolved
library/src/helpers/bindingsHelpers.ts Outdated Show resolved Hide resolved
library/src/containers/Bindings/BindingField.tsx Outdated Show resolved Hide resolved
library/src/containers/Bindings/Bindings.tsx Outdated Show resolved Hide resolved
@195858
Copy link
Contributor Author

195858 commented Mar 9, 2021

Hi,
I've made the changes you suggested except for the "bindingsHelper.isObject" method.

@195858 195858 requested a review from magicmatatjahu March 10, 2021 09:15
@magicmatatjahu
Copy link
Member

@195858 Very good, but I have some problems with modified part related to messages/operations:

image

Could you fix styling (paddings) for header title here? In addition, make that the badge subscribe should be a few px away from the title (I think we use 6 or 12 in another part for that case). Please also fix it for publish badge.

image

I don't know from which part we have this blank content, but it should be removed.

image

Do you wanna to render the summary and description in toggle? If this is intentional, I would prefer to do this in followup PR and modify other parts including channels, schematic etc, because currently we have inconsistencies. In addition, as you can see, the styles don't look very good :)

Comment on lines 101 to 130
<section
className={bemClasses.element(`${className}-oneOf-${payloadType}`)}
>
<header
className={bemClasses.element(
`${className}-oneOf-${payloadType}-header`,
)}
>
<h3>
{isPublish && isSubscribe ? (
<Badge
type={
payloadType === PayloadType.PUBLISH
? BadgeType.PUBLISH
: BadgeType.SUBSCRIBE
}
/>
) : null}
<span>
{payloadType === PayloadType.PUBLISH
? ONE_OF_FOLLOWING_MESSAGES_PUBLISH_SINGLE_TEXT
: ONE_OF_FOLLOWING_MESSAGES_SUBSCRIBE_SINGLE_TEXT}
</span>
</h3>
</header>
{operation.summary && (
<div className={bemClasses.element(`${className}-description`)}>
<Markdown>{operation.summary}</Markdown>
</div>
)}
Copy link
Member

Choose a reason for hiding this comment

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

Why do you wanna render single message with styling for oneOf?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I simply reused the existing styles - I think the action to take here is to create separate styles.
I just wanted the message to be rendered similar to one-of because the previous rendering omitted information. If a channel had both subscribe and publish operations the result was really confusing.
Hence I added the help text and the summary if it exists.

@195858
Copy link
Contributor Author

195858 commented Mar 11, 2021

@195858 Very good, but I have some problems with modified part related to messages/operations:

image

Could you fix styling (paddings) for header title here? In addition, make that the badge subscribe should be a few px away from the title (I think we use 6 or 12 in another part for that case). Please also fix it for publish badge.

Will do.

image

I don't know from which part we have this blank content, but it should be removed.

It's because the message does have neither a description nor a summary.
I'll see how I can omit this in this case.

image

Do you wanna to render the summary and description in toggle? If this is intentional, I would prefer to do this in followup PR and modify other parts including channels, schematic etc, because currently we have inconsistencies. In addition, as you can see, the styles don't look very good :)

OK, the toggle was there before - however I see that the description is now presented in the collapsed view. I'll address this

@195858
Copy link
Contributor Author

195858 commented Mar 11, 2021

image

Do you wanna to render the summary and description in toggle? If this is intentional, I would prefer to do this in followup PR and modify other parts including channels, schematic etc, because currently we have inconsistencies. In addition, as you can see, the styles don't look very good :)

A further remark - summary was rendered as part fo the header before. I don;t think I changed it - however I'll clean it up/find a better looking way.

@magicmatatjahu
Copy link
Member

@195858 Thank you very much for your work! I appreciate that. Please ping me when you done :)

@195858
Copy link
Contributor Author

195858 commented Mar 19, 2021

@magicmatatjahu - made some changes ...

The channel operation has a new header format. Operation binding and messages section is now indented so it's easier to see how the sections relate.
image

The top level message section is adjusted as well
image

@magicmatatjahu
Copy link
Member

magicmatatjahu commented Mar 22, 2021

@195858 Thanks for patience and for your work! I wanted to see it in my local machine, but as I see, styles aren't update from 14 days. Do you could push changes for new styling described in your last comment? I like the styles very much :)

@sonarqubecloud
Copy link

Kudos, SonarCloud 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
No Duplication information No Duplication information

@195858
Copy link
Contributor Author

195858 commented Mar 22, 2021

@magicmatatjahu - sorry forgot to push my changes.

Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

Choose a reason for hiding this comment

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

LGTM! :) Thanks for contribution!

@magicmatatjahu
Copy link
Member

@195858 I forgot that non-maintainers hasn't access to merge... Very, very sorry for delay and thanks again for contribution!

@magicmatatjahu magicmatatjahu merged commit b85715d into asyncapi:master Mar 23, 2021
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 0.21.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@derberg
Copy link
Member

derberg commented Mar 24, 2021

@all-contributors please add @195858 for code

@allcontributors
Copy link
Contributor

@derberg

I've put up a pull request to add @195858! 🎉

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

Successfully merging this pull request may close these issues.

4 participants