WIP - Improve org charts #3135
WIP - Improve org charts #3135
Conversation
…mprove-org-charts
…cy as well as fixes a bug
VassilIordanov
force-pushed
the
GH-3134-improve-org-charts
branch
from
a74076d
to
1e0300c
Compare
… index Also, make head positions bigger and limit depth of chart
|
This pull request introduces 1 alert when merging c72f74e into 05d6382 - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging e00a0e9 into 05d6382 - view on LGTM.com new alerts:
|
VassilIordanov
force-pushed
the
GH-3134-improve-org-charts
branch
from
e00a0e9
to
975d21c
Compare
|
This pull request introduces 1 alert when merging 975d21c into a2500d4 - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging 90f46a9 into 228f600 - view on LGTM.com new alerts:
|
| x={size[1] + 2} | ||
| y={size[1] / 2} | ||
| fontSize="9px" | ||
| fontWeight={twoRows ? "bold" : "normal"} |
There was a problem hiding this comment.
Please fix this warning:
| minimized: PropTypes.bool, | ||
| onMouseEnter: PropTypes.func, | ||
| onMouseLeave: PropTypes.func | ||
| } | ||
|
|
||
| const OrganizationalChart = ({ |
There was a problem hiding this comment.
I think it would be better to place top level component at the top to the lowest at the bottom, so that we don't scroll all the way to skim a file and see what is going on, high level picture, props etc. For example, I think we should structure our components in a file like this:
imports ...
TopLevelComp = ...
MidLevelComps = ...
LowLevelComps = ...
| content={ | ||
| <> | ||
| <LinkTo modelType="Person" model={position.person} /> -{" "} | ||
| <LinkTo modelType="Position" model={position} />{" "} |
There was a problem hiding this comment.
The last {" "} seems unnecessary
| (size[0] - size[1]) * 0.17 | ||
| )} | ||
| </Text> | ||
| {twoRows && ( |
There was a problem hiding this comment.
We already check this value to be true, twoRows will always be true below
| } | ||
| } | ||
| } | ||
| ` | ||
| const transitionDuration = 200 | ||
|
|
||
| const AVATAR_SIZE = 16 | ||
|
|
||
| const ranks = Settings.fields.person.ranks.map(rank => rank.value) | ||
|
|
There was a problem hiding this comment.
Actually this is about line:71, this is the code that i am refering to:
ranks.indexOf(p1.person?.rank) > ranks.indexOf(p2.person?.rank) ? -1 : 1
We could simplify this like this:
ranks.indexOf(p2.person?.rank) - ranks.indexOf(p1.person?.rank)
Internals:
Release notes
Closes #
User changes
Super User changes
Admin changes
System admin changes
Checklist
repo#issue: Titletitle format and these 7 rules