From a48b23e6a5aee5831f777e1cacfc55f4ad3afbd4 Mon Sep 17 00:00:00 2001 From: Rucha Mahabal Date: Thu, 8 Jul 2021 09:53:31 +0530 Subject: [PATCH] perf: Optimise Rendering - optimise get_children function - use promises instead of callbacks - optimise selectors - use const wherever possible - use pure js instead of jquery for connectors for faster rendering --- .../organizational_chart.py | 22 ++--- .../hierarchy_chart_desktop.js | 84 +++++++++---------- .../hierarchy_chart/hierarchy_chart_mobile.js | 65 +++++++------- 3 files changed, 79 insertions(+), 92 deletions(-) diff --git a/erpnext/hr/page/organizational_chart/organizational_chart.py b/erpnext/hr/page/organizational_chart/organizational_chart.py index 77b8df7520..46578f3aaf 100644 --- a/erpnext/hr/page/organizational_chart/organizational_chart.py +++ b/erpnext/hr/page/organizational_chart/organizational_chart.py @@ -2,33 +2,25 @@ from __future__ import unicode_literals import frappe @frappe.whitelist() -def get_children(parent=None, company=None, exclude_node=None, is_root=False, is_tree=False, fields=None): +def get_children(parent=None, company=None): filters = [['status', '!=', 'Left']] if company and company != 'All Companies': filters.append(['company', '=', company]) - if not fields: - fields = ['employee_name as name', 'name as id', 'reports_to', 'image', 'designation as title'] - - if is_root: - parent = '' - - if exclude_node: - filters.append(['name', '!=', exclude_node]) - if parent and company and parent != company: filters.append(['reports_to', '=', parent]) else: filters.append(['reports_to', '=', '']) - employees = frappe.get_list('Employee', fields=fields, - filters=filters, order_by='name') + employees = frappe.get_list('Employee', + fields=['employee_name as name', 'name as id', 'reports_to', 'image', 'designation as title'], + filters=filters, + order_by='name' + ) for employee in employees: - is_expandable = frappe.get_all('Employee', filters=[ - ['reports_to', '=', employee.get('id')] - ]) + is_expandable = frappe.db.count('Employee', filters={'reports_to': employee.get('id')}) employee.connections = get_connections(employee.id) employee.expandable = 1 if is_expandable else 0 diff --git a/erpnext/public/js/hierarchy_chart/hierarchy_chart_desktop.js b/erpnext/public/js/hierarchy_chart/hierarchy_chart_desktop.js index e89a98ac4f..bf366792a9 100644 --- a/erpnext/public/js/hierarchy_chart/hierarchy_chart_desktop.js +++ b/erpnext/public/js/hierarchy_chart/hierarchy_chart_desktop.js @@ -7,7 +7,6 @@ erpnext.HierarchyChart = class { - this method should return id, name, title, image, and connections for each node */ constructor(doctype, wrapper, method) { - this.wrapper = $(wrapper); this.page = wrapper.page; this.method = method; this.doctype = doctype; @@ -61,6 +60,8 @@ erpnext.HierarchyChart = class { frappe.breadcrumbs.add('HR'); let me = this; + if ($(`[data-fieldname="company"]`).length) return; + let company = this.page.add_field({ fieldtype: 'Link', options: 'Company', @@ -131,32 +132,30 @@ erpnext.HierarchyChart = class { method: me.method, args: { company: me.company - }, - callback: function(r) { - if (r.message.length) { - let nodes = r.message; - let node = undefined; - let first_root = undefined; - - $.each(nodes, (i, data) => { - node = new me.Node({ - id: data.id, - parent: $('
  • ').appendTo(me.$hierarchy.find('.node-children')), - parent_id: undefined, - image: data.image, - name: data.name, - title: data.title, - expandable: true, - connections: data.connections, - is_root: true - }); - - if (i == 0) - first_root = node; + } + }).then(r => { + if (r.message.length) { + let node = undefined; + let first_root = undefined; + + $.each(r.message, (i, data) => { + node = new me.Node({ + id: data.id, + parent: $('
  • ').appendTo(me.$hierarchy.find('.node-children')), + parent_id: undefined, + image: data.image, + name: data.name, + title: data.title, + expandable: true, + connections: data.connections, + is_root: true }); - me.expand_node(first_root); - } + if (i == 0) + first_root = node; + }); + + me.expand_node(first_root); } }); } @@ -204,18 +203,14 @@ erpnext.HierarchyChart = class { } get_child_nodes(node_id) { - let me = this; return new Promise(resolve => { frappe.call({ method: this.method, args: { parent: node_id, - company: me.company - }, - callback: (r) => { - resolve(r.message); + company: this.company } - }); + }).then(r => resolve(r.message)); }); } @@ -266,27 +261,28 @@ erpnext.HierarchyChart = class { } add_connector(parent_id, child_id) { + // using pure javascript for better performance const parent_node = document.querySelector(`#${parent_id}`); const child_node = document.querySelector(`#${child_id}`); let path = document.createElementNS('http://www.w3.org/2000/svg', 'path'); // we need to connect right side of the parent to the left side of the child node - let pos_parent_right = { + const pos_parent_right = { x: parent_node.offsetLeft + parent_node.offsetWidth, y: parent_node.offsetTop + parent_node.offsetHeight / 2 }; - let pos_child_left = { + const pos_child_left = { x: child_node.offsetLeft - 5, y: child_node.offsetTop + child_node.offsetHeight / 2 }; - let connector = this.get_connector(pos_parent_right, pos_child_left); + const connector = this.get_connector(pos_parent_right, pos_child_left); path.setAttribute('d', connector); this.set_path_attributes(path, parent_id, child_id); - $('#connectors').append(path); + document.getElementById('connectors').appendChild(path); } get_connector(pos_parent_right, pos_child_left) { @@ -330,12 +326,13 @@ erpnext.HierarchyChart = class { set_path_attributes(path, parent_id, child_id) { path.setAttribute("data-parent", parent_id); path.setAttribute("data-child", child_id); + const parent = $(`#${parent_id}`); - if ($(`#${parent_id}`).hasClass('active')) { + if (parent.hasClass('active')) { path.setAttribute("class", "active-connector"); path.setAttribute("marker-start", "url(#arrowstart-active)"); path.setAttribute("marker-end", "url(#arrowhead-active)"); - } else if ($(`#${parent_id}`).hasClass('active-path')) { + } else if (parent.hasClass('active-path')) { path.setAttribute("class", "collapsed-connector"); path.setAttribute("marker-start", "url(#arrowstart-collapsed)"); path.setAttribute("marker-end", "url(#arrowhead-collapsed)"); @@ -343,8 +340,9 @@ erpnext.HierarchyChart = class { } set_selected_node(node) { - // remove .active class from the current node - $('.active').removeClass('active'); + // remove active class from the current node + if (this.selected_node) + this.selected_node.$link.removeClass('active'); // add active class to the newly selected node this.selected_node = node; @@ -411,9 +409,9 @@ erpnext.HierarchyChart = class { } remove_levels_after_node(node) { - let level = $(`#${node.id}`).parent().parent().parent(); + let level = $(`#${node.id}`).parent().parent().parent().index(); - level = $('.hierarchy > li:eq('+ level.index() + ')'); + level = $('.hierarchy > li:eq('+ level + ')'); level.nextAll('li').remove(); let nodes = level.find('.node-card'); @@ -431,8 +429,8 @@ erpnext.HierarchyChart = class { remove_orphaned_connectors() { let paths = $('#connectors > path'); $.each(paths, (_i, path) => { - let parent = $(path).data('parent'); - let child = $(path).data('child'); + const parent = $(path).data('parent'); + const child = $(path).data('child'); if ($(`#${parent}`).length && $(`#${child}`).length) return; diff --git a/erpnext/public/js/hierarchy_chart/hierarchy_chart_mobile.js b/erpnext/public/js/hierarchy_chart/hierarchy_chart_mobile.js index 5eee27b5fc..17062e2585 100644 --- a/erpnext/public/js/hierarchy_chart/hierarchy_chart_mobile.js +++ b/erpnext/public/js/hierarchy_chart/hierarchy_chart_mobile.js @@ -7,7 +7,6 @@ erpnext.HierarchyChartMobile = class { - this method should return id, name, title, image, and connections for each node */ constructor(doctype, wrapper, method) { - this.wrapper = $(wrapper); this.page = wrapper.page; this.method = method; this.doctype = doctype; @@ -63,6 +62,8 @@ erpnext.HierarchyChartMobile = class { frappe.breadcrumbs.add('HR'); let me = this; + if ($(`[data-fieldname="company"]`).length) return; + let company = this.page.add_field({ fieldtype: 'Link', options: 'Company', @@ -139,24 +140,21 @@ erpnext.HierarchyChartMobile = class { args: { company: me.company }, - callback: function(r) { - if (r.message.length) { - let nodes = r.message; - - $.each(nodes, (_i, data) => { - return new me.Node({ - id: data.id, - parent: me.$hierarchy.find('.root-level'), - parent_id: undefined, - image: data.image, - name: data.name, - title: data.title, - expandable: true, - connections: data.connections, - is_root: true - }); + }).then(r => { + if (r.message.length) { + $.each(r.message, (_i, data) => { + return new me.Node({ + id: data.id, + parent: me.$hierarchy.find('.root-level'), + parent_id: undefined, + image: data.image, + name: data.name, + title: data.title, + expandable: true, + connections: data.connections, + is_root: true }); - } + }); } }); } @@ -237,11 +235,8 @@ erpnext.HierarchyChartMobile = class { parent: node_id, company: me.company, exclude_node: exclude_node - }, - callback: (r) => { - resolve(r.message); } - }); + }).then(r => resolve(r.message)); }); } @@ -286,10 +281,10 @@ erpnext.HierarchyChartMobile = class { } add_connector(parent_id, child_id) { - let parent_node = document.querySelector(`#${parent_id}`); - let child_node = document.querySelector(`#${child_id}`); + const parent_node = document.querySelector(`#${parent_id}`); + const child_node = document.querySelector(`#${child_id}`); - let path = document.createElementNS('http://www.w3.org/2000/svg', 'path'); + const path = document.createElementNS('http://www.w3.org/2000/svg', 'path'); let connector = undefined; @@ -299,10 +294,10 @@ erpnext.HierarchyChartMobile = class { connector = this.get_connector_for_collapsed_node(parent_node, child_node); } - path.setAttribute("d", connector); + path.setAttribute('d', connector); this.set_path_attributes(path, parent_id, child_id); - $('#connectors').append(path); + document.getElementById('connectors').appendChild(path); } get_connector_for_active_node(parent_node, child_node) { @@ -351,19 +346,21 @@ erpnext.HierarchyChartMobile = class { set_path_attributes(path, parent_id, child_id) { path.setAttribute("data-parent", parent_id); path.setAttribute("data-child", child_id); + const parent = $(`#${parent_id}`); - if ($(`#${parent_id}`).hasClass('active')) { + if (parent.hasClass('active')) { path.setAttribute("class", "active-connector"); path.setAttribute("marker-start", "url(#arrowstart-active)"); path.setAttribute("marker-end", "url(#arrowhead-active)"); - } else if ($(`#${parent_id}`).hasClass('active-path')) { + } else if (parent.hasClass('active-path')) { path.setAttribute("class", "collapsed-connector"); } } set_selected_node(node) { // remove .active class from the current node - $('.active').removeClass('active'); + if (this.selected_node) + this.selected_node.$link.removeClass('active'); // add active class to the newly selected node this.selected_node = node; @@ -494,9 +491,9 @@ erpnext.HierarchyChartMobile = class { } remove_levels_after_node(node) { - let level = $(`#${node.id}`).parent().parent(); + let level = $(`#${node.id}`).parent().parent().index(); - level = $('.hierarchy-mobile > li:eq('+ (level.index()) + ')'); + level = $('.hierarchy-mobile > li:eq('+ level + ')'); level.nextAll('li').remove(); let current_node = level.find(`#${node.id}`); @@ -512,8 +509,8 @@ erpnext.HierarchyChartMobile = class { remove_orphaned_connectors() { let paths = $('#connectors > path'); $.each(paths, (_i, path) => { - let parent = $(path).data('parent'); - let child = $(path).data('child'); + const parent = $(path).data('parent'); + const child = $(path).data('child'); if ($(`#${parent}`).length && $(`#${child}`).length) return;