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

Organization Chart #2726

Closed
wants to merge 7 commits into from
Closed

Conversation

bkrith
Copy link
Contributor

@bkrith bkrith commented May 1, 2022

Fixes #2730

Overview

Basic organization chart for contacts.
Used d3 and d3-org-chart libraries.

Requirements

Have contacts with Org Manager field.
And, the root contact with Org Manager set to "Head".
image

Add field orgmanager in contact

image

Organization chart

image

Highlight path to root

image

Todo

  • What is the element in the top right, are there different org charts to switch between? Is this something we want to directly have in version 1, and if so how many charts are there usually? (Cause I would assume it would be just one?)
  • The background color would look better as color-main-background instead of grey
  • Currently there are a lot of inline styles. This results in the app not working with dark theme, and it also being difficult to change. Best is to use the variables supplied by Nextcloud :)
  • Currently everything is divs without semantics, which is not accessible. For example, the name of people need to be h3, the title and the info lines below p
  • The placeholder icon for people without avatar is not Nextcloud standard – if you use NcAvatar you will automatically get placeholders https://nextcloud-vue-components.netlify.app/#/Components/NcAvatar
  • The blue bar on top of each person looks nice – but does not seem to be taken from var(--color-primary) – would be better to do that. Also the whitespace above that bar can be removed.
  • The highlight path too root should also be color-primary, and to make it more obvious you can use a border of 2px. Also the border is not around the existing box, there’s another container around it?
  • The default border should be border: 1px solid var(--color-border) using the variable
  • You can use the border radius value var(--border-radius-large) on the containers
  • Manages/oversees uses custom small text sizes which are not accessible. Just use the default font size / no need to change it :)
  • For people who do not manage or oversee anyone, that part does not need to be shown at all
  • The icons next to manages/oversees introduce visual noise and are not needed, can be removed :)
  • BUG new avatars are transparent Fix non-user avatar on complex background nextcloud-libraries/nextcloud-vue#3272
    Bildschirmfoto vom 2022-09-19 14-45-01
  • BUG missing handling of deleted managers
    Bildschirmfoto vom 2022-09-19 14-54-16
  • BUG cyclic references (alice is bob's manager, bob is alice's manager) show blank chart page with empty list of charts
    Bildschirmfoto vom 2022-09-19 14-55-41
  • BUG graph element is not responsive. Enlarging the window causes partly hidden graphs.
    Bildschirmfoto vom 2022-09-19 15-01-24
    Bildschirmfoto vom 2022-09-19 15-01-36
  • Graph card link says https://localhost/apps/contacts/chart/All%20contacts but click on avatar gets me to https://localhost/apps/contacts/All%20contacts/4cee56a8-c43c-4e4a-bcba-55026faf1687~contacts. This means a ctrl+click (or right click and open in new tab) leads to a different page than an ordinary click
  • To research (for the lack of reproducibility at the moment): handling of shared address books and cross-addressbook references in general. I guess managers should only be assignable within the same addressbook so that we can assure there are no broken links.
  • Show more levels by default -> all levels?
  • Order chart dropdown by alphabet Sort org charts #2973
  • Hide chart dropdown when there is only one option
  • Expand button is rather small
  • Height of card header is not even -> some cards have a bigger height

Signed-off-by: Vassilis Kritharakis <bkrith@hotmail.com>
@bkrith bkrith changed the title orgChart for contacts - basic mvp orgChart for contacts May 1, 2022
Signed-off-by: Vassilis Kritharakis <bkrith@hotmail.com>
@ChristophWurst

This comment was marked as outdated.

Signed-off-by: Vassilis Kritharakis <bkrith@hotmail.com>
@bkrith bkrith changed the title orgChart for contacts Organization Chart May 2, 2022
@florisvangeel
Copy link

This PR is the result of an idea proposal at NextGov 2022 Hackathon

Abstract:
In contacts there is one special circle with the company directory.
This helper app will create two relations from this contact to its manager and side reporter.
With that information the company directory can get a live organisational chart of all the people.
Using an open source library
https://www.npmjs.com/package/d3-org-chart

there might be some UX improvements needed. This challenge comes down to defining the datasets and building the first prototype within nextcloud.

As a result every team can be organised.

Presentation: https://docs.google.com/presentation/d/1r_J-nop5r-cgDtSQpHPNbwJ5igaCmftc/
Demo video: https://www.youtube.com/watch?v=vM_2yXlhbak

@codecov

This comment was marked as outdated.

@bkrith bkrith mentioned this pull request May 4, 2022
@dinesh-dot-com
Copy link

pls help me to build org chart

Copy link

@dinesh-dot-com dinesh-dot-com left a comment

Choose a reason for hiding this comment

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

not working for me

@bkrith
Copy link
Contributor Author

bkrith commented Aug 15, 2022

I'm sorry for the delay.. I was busy with my dayjob..

@DineshRefex maybe it was a confusion with HEAD value which needed for the top member(root) of chart.

Please try again I have removed this value(exists but it's not necessary anymore) and also added support for multiple charts.

@bkrith
Copy link
Contributor Author

bkrith commented Aug 15, 2022

Dropdown at the right top with all available charts generated from contacts.
For now the name of chart is the org field of root contact and the fullName.
image

@dinesh-dot-com
Copy link

dinesh-dot-com commented Aug 16, 2022 via email

@AndyScherzinger AndyScherzinger added enhancement New feature or request 3. to review Waiting for reviews skill:backend Issues and PRs that require backend development skills skill:frontend Issues and PRs that require JavaScript/Vue/styling development skills labels Aug 18, 2022
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

It works

Bildschirmfoto vom 2022-08-18 14-39-11

l10n/el.js Outdated Show resolved Hide resolved
src/components/OrgChart.vue Outdated Show resolved Hide resolved
src/components/OrgChart.vue Outdated Show resolved Hide resolved
* @memberof Contact
*/
get orgManager() {
return this.firstIfArray(this.vCard.getFirstPropertyValue('orgmanager'))
Copy link
Member

Choose a reason for hiding this comment

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

is this possibly a standardized property? I'm not able to find references of ORGMANAGER in the vcard specs. If it's not, should be prefix with X- @skjnldsv?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, if not official, prefix is mandatory

Copy link
Member

@CarlSchwan CarlSchwan Aug 18, 2022

Choose a reason for hiding this comment

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

If you use X-ManagersName, it will be compatible with korganizer/kalendar, better than nothing 😜

A similar concept exist in google eith manager and assistant name but they don't even use vcards

Copy link
Member

Choose a reason for hiding this comment

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

We already have "Relationship to you" with "Manager", "Colleague" and "Assistant", is that not a standard of sorts we can use here too?
image

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 just pushed commit with all the requested changes/bugs/etc.
Currently I'm using X-ManagersName.. Relationship field is like first person property and I'm not sure(never used) if is anything else except an info.. except if extend/change it..

Copy link
Member

Choose a reason for hiding this comment

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

Relationship to you is not helpful because it only stores the type of relation and is always relative to the addressbook owner. E.g. in your addressbook there might be a contact for me and it says Colleague. Nothing more, nothing less.

X-ManagersName seems a lot more appropriate. The only issue we just found out is that KOrganizer stores the name of the other person for this field, not the UID. So it is not a reference to another contact. It is just a value. And this value is prone for conflicts in larger organizations where names are not unique.

I'll see if we can put together some kind of hybrid solution where we store the UID for our purposes but also X-ManagersName for interoperability.

Copy link
Member

Choose a reason for hiding this comment

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

We will go with something like X-MANAGERSNAME;UID=abc-def-123:John

@ChristophWurst

This comment was marked as resolved.

@ChristophWurst

This comment was marked as resolved.

@ChristophWurst

This comment was marked as resolved.

@ChristophWurst

This comment was marked as resolved.

@ChristophWurst

This comment was marked as resolved.

@ChristophWurst

This comment was marked as resolved.

src/components/AppContent/ChartContent.vue Outdated Show resolved Hide resolved
* @memberof Contact
*/
get orgManager() {
return this.firstIfArray(this.vCard.getFirstPropertyValue('orgmanager'))
Copy link
Member

@CarlSchwan CarlSchwan Aug 18, 2022

Choose a reason for hiding this comment

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

If you use X-ManagersName, it will be compatible with korganizer/kalendar, better than nothing 😜

A similar concept exist in google eith manager and assistant name but they don't even use vcards

@ChristophWurst ChristophWurst added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Sep 1, 2022
@bkrith
Copy link
Contributor Author

bkrith commented Sep 11, 2022

Property "Manager" based on X-ManagersName.
image

Re-aligned and improved dropdown for multiple charts.
image

align nodes styles and highlight path according color guidelines.
image

@ChristophWurst

This comment was marked as resolved.

@ChristophWurst
Copy link
Member

ChristophWurst commented Sep 16, 2022

  • BUG I can no longer select a manager. Possibly related to the UI update because contactsOptions does contain data.

Bildschirmfoto vom 2022-09-16 17-57-26

@ChristophWurst
Copy link
Member

BUG I can no longer select a manager. Possibly related to the UI update because contactsOptions does contain data.

I tried again with the outdated branch. Same problem. However, once I created a third contact I was able to define managers. Bug?!

@ChristophWurst
Copy link
Member

ChristophWurst commented Sep 19, 2022

Today's state with the new Nextcloud 25 UI

Bildschirmfoto vom 2022-09-19 15-13-04

@ChristophWurst
Copy link
Member

ChristophWurst commented Sep 19, 2022

@bkrith I have checked my previously commented bugs against the current state. Great job. Those are all resolved. I haven't had the chance to also check Jan's design feedback. Since you don't have permission to edit other people's comments and markdown checklist, I moved the todos up into your PR description so you can check the items and mark everything that's already done.

I will also add any bugs to the todo list as I encounter them.

Edit: design feedback checked against the list.

@ChristophWurst
Copy link
Member

I'd say let's get the remaining bugs fixed, then we can merge a first version of this feature. Anything else can be done as follow-ups.

@ChristophWurst ChristophWurst added this to the v5.0 milestone Sep 20, 2022
name: 'chart',
params: { selectedChart: GROUP_ALL_CONTACTS },
}"
icon="icon-category-monitoring" />
Copy link
Member

Choose a reason for hiding this comment

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

icon classes are deprecated. this should use a material icon

@@ -281,6 +292,11 @@ export default {
return this.sortedContacts.filter(contact => this.contacts[contact.key].groups && this.contacts[contact.key].groups.length === 0)
},

// check if any contact has manager, if not then is no need for organization chart menu
existChart() {
return !!Object.keys(this.contacts).filter(key => this.contacts[key].managersName).length
Copy link
Member

Choose a reason for hiding this comment

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

@@ -41,7 +41,7 @@ const isEmpty = value => {
export const ContactKindProperties = ['KIND', 'X-ADDRESSBOOKSERVER-KIND']

export const MinimalContactProperties = [
'EMAIL', 'UID', 'CATEGORIES', 'FN', 'ORG', 'N', 'X-PHONETIC-FIRST-NAME', 'X-PHONETIC-LAST-NAME',
'EMAIL', 'UID', 'CATEGORIES', 'FN', 'ORG', 'N', 'X-PHONETIC-FIRST-NAME', 'X-PHONETIC-LAST-NAME', 'X-MANAGERSNAME', 'TITLE',
Copy link
Member

Choose a reason for hiding this comment

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

what are the consequences of this change? I'm not able to figure that out from the code

@ChristophWurst
Copy link
Member

@bkrith if you allow us to push commits to your branch we can fix some of the reported problems right away

@ChristophWurst
Copy link
Member

I'll open a second PR with our branch

@bkrith
Copy link
Contributor Author

bkrith commented Sep 22, 2022

Thank you! I'm too busy right now.. I'm moving on new apartment and I have overloaded in my day job.

name: 'contact',
params: {
selectedGroup: this.$route.params.selectedChart,
selectedContact: `${uid}~contacts`,
Copy link
Member

Choose a reason for hiding this comment

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

I was wondering why my chart->contact links don't work anymore. turns out my addressbook just has a different name 🙈

@ChristophWurst
Copy link
Member

#2954 is ready for review and test

@dinesh-dot-com
Copy link

dinesh-dot-com commented Oct 11, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress enhancement New feature or request skill:backend Issues and PRs that require backend development skills skill:frontend Issues and PRs that require JavaScript/Vue/styling development skills
Projects
Development

Successfully merging this pull request may close these issues.

Organization Chart
8 participants