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: list all messages in sidebar #31

Merged
merged 3 commits into from
Jun 2, 2020
Merged

feat: list all messages in sidebar #31

merged 3 commits into from
Jun 2, 2020

Conversation

WaleedAshraf
Copy link
Contributor

closes #29

Updated view:
image

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.

lgtm

just please remember that we no longer create branches in the upstream and everyone should contribute through forks -> https://github.com/asyncapi/.github/blob/master/git-workflow.md

I'm using this script to setup master in fork to be in sync with master from upstream, so experience of working in fork is same awesome like working directly in the upstream.

@derberg derberg changed the title [Issue-29] List all messages in sidebar feat: list all messages in sidebar May 26, 2020
@derberg
Copy link
Member

derberg commented May 26, 2020

@WaleedAshraf and we also use conventional commits to drive releases automation https://github.com/asyncapi/.github/blob/master/CONTRIBUTING.md#conventional-commits
I already changed the PR title for you

@derberg
Copy link
Member

derberg commented May 26, 2020

I'm wondering if this is not something that should be behind a parameter so users can decide if they want it or not, but yeah, let us add it by default and see what community has to say on it

@derberg
Copy link
Member

derberg commented May 26, 2020

@WaleedAshraf one more question, you are aware that one message can be used in multiple channels and as a result, like in the example, link like <a name="message-turnOnOff"></a> gets duplicated, and message link will work only with a first occurrence in the list. Shouldn't there messages be rendered separately like here https://asyncapi.github.io/asyncapi-react/ ? and links from navigation point to this separate section with messages only?

@WaleedAshraf
Copy link
Contributor Author

@bpedro you are right. There can be multiple links for one message, and this fix will only take you to the first link.
My understanding is that in the future we can have filters. And when someone clicks a message, we apply a filter and only show messages which have that schema.

Using filters can also be useful for tags. i.e, we list all the tags, and clicking on one tag will filter all messages which have that tag. etc.

The way you showed above will also work. For that first, we need to

  • make schema collapses able
  • render messages, servers, schema in separate sections

@derberg
Copy link
Member

derberg commented May 26, 2020

@WaleedAshraf this filter functionality if pretty sophisticated, do you plan to work on it?

with this PR schema doesn't have to be collapsable, and you would only have to render messages (servers or schemas are not part of this PR) below operations, and link to messages there only where you do not have duplicates, instead of a message in a given operation.

@WaleedAshraf
Copy link
Contributor Author

WaleedAshraf commented May 26, 2020

Ok, I'll work on rending messages in below Operation and linking it there. 👍

Work on Filter: it's just an idea. maybe there are better ways to do it.

@WaleedAshraf
Copy link
Contributor Author

@derberg

Kapture 2020-06-01 at 19 31 27

@@ -5,3 +5,4 @@
{% if asyncapi.hasChannels() %}
{% include "./operations.html" %}
{% endif %}
{% include "./messages.html" %}
Copy link
Contributor Author

@WaleedAshraf WaleedAshraf Jun 1, 2020

Choose a reason for hiding this comment

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

It would be good to have .hasMessages() check here. But it doesn't exist yet.
We can add that in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Please create an issue in parser to add it so we don't forget

@derberg
Copy link
Member

derberg commented Jun 2, 2020

@WaleedAshraf lgtm, just create an issue for .hasMessage()
I guess people might want to have examples next to those messages but yeah, let community request to make sure they need it

@derberg derberg merged commit 33be7db into master Jun 2, 2020
@derberg derberg deleted the issue-29 branch June 2, 2020 10:56
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 0.9.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@rv0
Copy link

rv0 commented Jun 18, 2020

I'm wondering if this is not something that should be behind a parameter so users can decide if they want it or not, but yeah, let us add it by default and see what community has to say on it

oh boy :)
our api is quite extensive and most payloads aren't named (sometimes we should and eventually will, but usually it does not make sense).. so currently we have like 250+ "<anonymous-message-xyz>" entries shown in the sidebar

In our case, having these entries in the sidebar offers no added value, even if they were all named, there's simply way too much of them to have any use.

@derberg
Copy link
Member

derberg commented Jun 18, 2020

@rv0 thanks for sharing, we have use case, makes sense to do it. Please create a separate issue. Adding param is easy, we just need to agree on how to name it and what is the default.

btw do you know that you can lock for a specific html-template version so you are not surprised with new features you might not want to have? :)

@rv0
Copy link

rv0 commented Jun 18, 2020

@derberg will create issue eventually, I'm aware of the version targeting, it's great how fast this project and it's subprojects are advancing
it's just a bit weird that features are released without a way to configure them (as you say yourself, adding param is easy), hence my reply here

@derberg
Copy link
Member

derberg commented Jun 18, 2020

@rv0 please keep in mind that this template is not yet on 1.0 release hence the approach is more relaxed here. It would be great though to work together on some design document that outlines what features should be parametrized and what not, or maybe really all.

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.

list all messages in side bar
4 participants