-
Notifications
You must be signed in to change notification settings - Fork 33
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
[DOC] Improve readme #643
[DOC] Improve readme #643
Conversation
d7bb984
to
af9d0aa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Various things to discuss
README.md
Outdated
``` | ||
* Define your bpmn content | ||
``` | ||
const bpmnContent = `<?xml version="1.0" encoding="UTF-8"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as an end user, I prefer not having to do such things. We should guide them about how they can do this in a better way, probably when having done process-analytics/bpmn-visualization-examples#18
In addition, we highlight bpmn-js/bpmn.io
in the README whereas it is a kind of competitor for our lib
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change/remove some attributes of definitions
.
For the people who don't know what is BPMN or want a simple example to implement faster, I think it's important to have this.
When I'm a new end user, I want to have all the information to understang the base of the library, and start my poc easly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you about the need for BPMN guidance, but in practise, BPMN diagram are usually loaded from an external source.
We have examples that show how to do this, both from remote sources (from miwg-test-suite) or from the code.
I would prefer we explain user to go to the examples, this will let the usage paragraph focussed on the lib and not on how to fetch/retrieve xml content which is out of the direct scope of the lib.
Have a look at bpmn-js usage, this is how it is explained and IHMO this keep the usage clearer.
var xml; // my BPMN 2.0 xml
var viewer = new BpmnJS({....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
README.md
Outdated
[![GitHub release (latest by date including pre-releases)](https://img.shields.io/github/v/release/process-analytics/bpmn-visualization-js?color=orange&include_prereleases)](https://github.com/process-analytics/bpmn-visualization-js/releases) | ||
[![Build](https://github.com/process-analytics/bpmn-visualization-js/workflows/Build/badge.svg)](https://github.com/process-analytics/bpmn-visualization-js/actions) | ||
[![Live Demo](https://img.shields.io/badge/demo-online-blueviolet.svg)][demo-live-environment] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the demo badge
README.md
Outdated
[![GitHub release (latest by date including pre-releases)](https://img.shields.io/github/v/release/process-analytics/bpmn-visualization-js?color=orange&include_prereleases)](https://github.com/process-analytics/bpmn-visualization-js/releases) | ||
[![Build](https://github.com/process-analytics/bpmn-visualization-js/workflows/Build/badge.svg)](https://github.com/process-analytics/bpmn-visualization-js/actions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ we may probably put the build badge on its own line as it relates to the current build status
Demo and release relate to existing released materials
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid there will too many lines of badges after.
I can put the build badge after the demo badge.
|
||
## 🎮 Demo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
about the emoji, I am sure this make things clearer: at a first glance, this looks appealing but as we have a lot of paragraphs, it overloads the page IMHO.
But this is a matter of taste I guess, I won't veto on this at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find that, since we have a lot of paragraphs, we can find a paragraph quite easily if you scroll quickly.
We can always ask for the opinion of someone outside the project to decide ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
me perso I like the icons
f9bf5dc
to
3419a81
Compare
3293d20
to
915ed5d
Compare
add break line at the end of the intro and remove the one between the title and the intro
4a197bd
to
f10e384
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still not fully agree with the solution proposed in this PR, in particular changes in the html documentation.
But let's move on and change the doc again later
No description provided.