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

README for plexus #425

Merged
merged 7 commits into from
Aug 5, 2019
Merged

Conversation

tiffon
Copy link
Member

@tiffon tiffon commented Jul 29, 2019

Which problem is this PR solving?

Provides some information on the plexus package.

Short description of the changes

Added content to the readme.

Also, I revised the ordering of merging props so the default props (like { style: { position: 'relative' } } can be overridden via the setOn* hooks.

TODO

  • Most of the TODOs noted in the README (some are well suited to a different PR)

Signed-off-by: Joe Farro <joef@uber.com>
Signed-off-by: Joe Farro <joef@uber.com>
@tiffon tiffon added the plexus Specific to packages/plexus label Jul 29, 2019
@tiffon tiffon requested a review from everett980 July 29, 2019 05:55
@codecov
Copy link

codecov bot commented Jul 29, 2019

Codecov Report

Merging #425 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #425      +/-   ##
==========================================
+ Coverage   91.62%   91.65%   +0.02%     
==========================================
  Files         176      176              
  Lines        4012     4012              
  Branches      957      957              
==========================================
+ Hits         3676     3677       +1     
+ Misses        294      293       -1     
  Partials       42       42
Impacted Files Coverage Δ
...eViewer/TimelineHeaderRow/TimelineViewingLayer.tsx 89.83% <0%> (+1.69%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5159ae5...1353f71. Read the comment docs.

@tiffon tiffon mentioned this pull request Jul 31, 2019
Copy link
Collaborator

@everett980 everett980 left a comment

Choose a reason for hiding this comment

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

Only finished the README. I'm leaving this feedback first as I figure this will be the part with the most discussion.

packages/plexus/README.md Outdated Show resolved Hide resolved
packages/plexus/README.md Show resolved Hide resolved
packages/plexus/README.md Outdated Show resolved Hide resolved
packages/plexus/README.md Show resolved Hide resolved
packages/plexus/README.md Show resolved Hide resolved
packages/plexus/README.md Outdated Show resolved Hide resolved
packages/plexus/README.md Show resolved Hide resolved
packages/plexus/README.md Show resolved Hide resolved
packages/plexus/README.md Show resolved Hide resolved

## Recipes

TODO:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these (or some of these) things we think we'll tackle while doing DDG work? If so, can we add a bullet point to related stories to come back here and update this section?

Copy link
Member Author

Choose a reason for hiding this comment

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

The only catch is, for the border recipes they're already in the demo app, just need to add them to this section.

Signed-off-by: Joe Farro <joef@uber.com>
@tiffon
Copy link
Member Author

tiffon commented Aug 3, 2019

@everett980 This is ready for another look; I made changes per your feedbabck.

Copy link
Collaborator

@everett980 everett980 left a comment

Choose a reason for hiding this comment

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

found two typos but otherwise it LGTM

| measureNode | `TMeasureNodeFn` _See below for details on this type._ |
| | Overrides the default measuring of nodes.<br>&nbsp; |

sThe types for `setOnNode` and `renderNode` are distinct from the corresponding fields on a non-measurable nodes layer in that the first argument is the `TVertex`, and the `TLayoutVertex` argument is **only available after initial render**.
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's an extra s at the beginning

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. The CMD button on my mac is glitchy :/

| useDotEdges | `boolean = false` |
| | When `true` the dot edges are used; i.e. generating neato edge paths is skipped.<br>&nbsp; |
| splines | `string = "true"` |
| | GraphViz [splines](https://www.graphviz.org/doc/info/attrs.html#d:splines) graph attribute.<br>&nbsp; |
| sep | `number = 0.5` |
| | GraphViz [sep](https://www.graphviz.org/doc/info/attrs.html#d:sep) graph attribute.<br>&nbsp; |
| | GraphViz [sep](https://www.graphviz.org/doc/info/attrs.html#d:sep) graph attribute, which defines the space margin around nodes.s<br>&nbsp; |
Copy link
Collaborator

Choose a reason for hiding this comment

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

extra s at the end (nodes.s)

everett980 and others added 2 commits August 5, 2019 16:26
Signed-off-by: Joe Farro <joef@uber.com>
@tiffon tiffon merged commit 9df691d into jaegertracing:master Aug 5, 2019
@tiffon tiffon deleted the layered-digraph-readme branch August 5, 2019 21:00
@tiffon tiffon restored the layered-digraph-readme branch August 5, 2019 21:00
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 5, 2021
README for plexus
Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plexus Specific to packages/plexus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants