Add explore apps statistics component - Closes #4358 #4370

isalga · 2022-06-28T13:55:44Z

What was the problem?

This PR resolves #4358

How was it solved?

Create component
Add component to scaffolded page
Mock API call

How was it tested?

Add unit test

…-statistics

soroushm · 2022-07-01T08:18:04Z

src/modules/blockchainApplication/utils/api/index.js

+ *
+ * @returns {Promise}
+ */
+export const getStatistics = () => Promise.resolve({


add the end point and make the api call

We agreed to mock it and API call is not ready. It should be done after this ticket. LiskArchive/lisk-service#1113

I suggest creating a ticket to unmock the api call (Which is written but comented)

add the api call here but inside the component use the mock not the api response check @eniolam1000752 pr

soroushm · 2022-07-01T08:20:41Z

src/modules/blockchainApplication/explore/components/BlockchainApplicationStatistics/index.js

+    datasets: [
+      {
+        backgroundColor: [colorPalette[1], colorPalette[0], colorPalette[2]],
+        data: apps.data.reduce((acc, stats) => {


why? the api response it will give us the data

{ "data": { "registered": 2503, "active": 2328, "terminated": 35, “totalSupplyLSK”: “5000000”, “stakedLSK”: “3000000”, “inflationRate”: “4.50”, }, "meta": {}, "links": {} }

There are 2 callls, 1 for apps list and other for apps. We have to iterate the apps list to get the status and generate the status map. As far as I understood

please go over the end point and check it one more time,

api is providing the application count based on the status
"registered": 2503, "active": 2328, "terminated": 35,

soroushm · 2022-07-01T08:23:15Z

src/modules/blockchainApplication/manage/components/ManageBlockchainApplications/index.js

+import React from 'react';
+import BlockchainApplicationStatistics from '../../../explore/components/BlockchainApplicationStatistics';
+
+const ManageBlockchainAppications = ({ apps, statistics }) => (


please improve naming apps is so general, also it dosnt need it

How I should rename it?

…statistics

…into 4358-explore-apps-statistics

soroushm · 2022-07-05T08:04:32Z

src/modules/blockchainApplication/utils/api/index.js

+ *
+ * @returns {Promise}
+ */
+export const getApps = ({


remove unused apps

soroushm · 2022-07-05T08:05:13Z

src/modules/blockchainApplication/utils/api/index.js

@@ -0,0 +1,49 @@
+/* istanbul ignore file */


the file should be go to /blockchainApplication/explore/api

soroushm · 2022-07-05T08:08:10Z

src/modules/blockchainApplication/manage/manager/ManageBlockchainApplications/index.js

@@ -0,0 +1,25 @@
+/* istanbul ignore file */


please change the component file from index to BlockchainApplicationStatistics

soroushm · 2022-07-05T08:11:45Z

src/modules/blockchainApplication/explore/components/BlockchainApplicationStatistics/index.js

+  const doughnutChartData = {
+    labels: [
+      t('Registered'),
+      t('Active'),
+      t('Terminated'),
+    ],
+    datasets: [
+      {
+        backgroundColor: [colorPalette[1], colorPalette[0], colorPalette[2]],
+        data: [statistics.data.registered, statistics.data.active, statistics.data.terminated],
+      },
+    ],
+  };
+
+  const doughnutChartOptions = {
+    cutoutPercentage: 70,
+    legend: {
+      display: true,
+      position: 'left',
+      align: 'start',
+      labels: {
+        padding: 20,
+      },
+    },
+    layout: {
+      padding: {
+        left: 0,
+        right: 0,
+        bottom: 0,
+        top: 0,
+      },
+    },
+  };


please move the part to utils and keep the component clean

Suggested change

const doughnutChartData = {

labels: [

t('Registered'),

t('Active'),

t('Terminated'),

],

datasets: [

{

backgroundColor: [colorPalette[1], colorPalette[0], colorPalette[2]],

data: [statistics.data.registered, statistics.data.active, statistics.data.terminated],

},

],

};

const doughnutChartOptions = {

cutoutPercentage: 70,

legend: {

display: true,

position: 'left',

align: 'start',

labels: {

padding: 20,

},

},

layout: {

padding: {

left: 0,

right: 0,

bottom: 0,

top: 0,

},

},

};

const {doughnutChartData, doughnutChartOptions} = prepareChartDataAndOptions(data, colorPalette)

src/modules/blockchainApplication/explore/components/BlockchainApplicationStatistics/index.js

soroushm · 2022-07-05T08:17:49Z

src/modules/blockchainApplication/explore/components/BlockchainApplicationStatistics/index.js

+          <p className={`${styles.statsInfo} stacked-token`}>
+            <TokenAmount
+              val={toRawLsk(statistics.data.stakedLSK)}
+              token={tokenMap.LSK.key}


what is the LSK map here they are deprecated and non-related to here

Then how we pass the token? If we don't pass token it will display '5,000,000' instead of '5,000,000 LSK(or other token)' as it is in design

i just prefer to use a constant for those things, but now tokenMap is fine

eniolam1000752 · 2022-07-05T08:59:30Z

Please Resolve merge conflicts :)

…statistics

eniolam1000752 · 2022-07-05T10:29:07Z

src/modules/blockchainApplication/explore/api/index.js

+/* istanbul ignore file */
+import http from 'src/utils/api/http';
+import { HTTP_PREFIX } from 'src/const/httpCodes';
+
+const httpPaths = {
+  blockchainAppsStatistics: `${HTTP_PREFIX}/blockchain/apps/statistics`,
+};
+
+/**
+ * Retrieves sidechains statistics information
+ *
+ * @param {Object} data
+ * @param {Object} data.network The network config from the Redux store
+ *
+ * @returns {Promise}
+ */
+// eslint-disable-next-line import/prefer-default-export
+export const getStatistics = ({
+  network,
+  params = {},
+  baseUrl,
+}) => http({
+  path: httpPaths.blockchainAppsStatistics,
+  network,
+  params,
+  baseUrl,
+});


I think this needs a unit test

eniolam1000752 · 2022-07-05T10:32:07Z

...n/explore/components/BlockchainApplicationStatistics/blockchainApplicationStatistics.test.js

+
+    expect(wrapper.find('TokenAmount').at(0)).toHaveText('5,000,000');
+    expect(wrapper.find('TokenAmount').at(1)).toHaveText('3,000,000');
+  });


I think you can add an extra assertion to check if Total Supply and Staked text render properly

eniolam1000752 · 2022-07-05T10:32:49Z

src/modules/blockchainApplication/explore/utils/chartUtils.js

@@ -0,0 +1,38 @@
+// eslint-disable-next-line import/prefer-default-export
+export const prepareChartDataAndOptions = (statistics, colorPalette, t) => {


I think you can have a unit test for this function

eniolam1000752 · 2022-07-05T11:53:50Z

src/modules/blockchainApplication/explore/api/index.test.js

+const baseUrl = 'http://custom-base-url.com/';
+const { network } = getState();
+
+describe('get blockchain application detail', () => {


I think this should be

Suggested change

describe('get blockchain application detail', () => {

describe('get blockchain application statistics', () => {

eniolam1000752 · 2022-07-05T11:55:01Z

src/modules/blockchainApplication/manage/components/ManageBlockchainApplications/index.js

+  <>
+    <BlockchainApplicationStatistics apps={apps} statistics={statistics} />
+  </>


I don't see a need for fragments(<>) here

soroushm · 2022-07-05T11:58:25Z

src/modules/blockchainApplication/explore/utils/chartUtils.js

@@ -0,0 +1,38 @@
+// eslint-disable-next-line import/prefer-default-export


I prefer to have the file name equal to the function so its easier to find it,

* Add component and screen structure * Add tooltips * Create api util * Add TODO dependency on api call * Add unit test * Add icons * Adjust styles * Fix lint * Adjust chart styles and options * Add coverage expections * Increase e2e wait time * Add alias * remove old routesMap * PR feedback * Update unit test * PR feedback * Fix test and lint * Add unit tests * Ignore branch coverage * Lint issues * PR feedback

Add component and screen structure

5b04b39

isalga self-assigned this Jun 28, 2022

isalga changed the base branch from development to feature/blockchain-agnostic-new June 28, 2022 13:55

iris salcedo added 12 commits June 28, 2022 16:00

Add tooltips

cbcb459

Create api util

63d457d

Add TODO dependency on api call

7584902

Add unit test

2f80124

Add icons

1f6967c

Adjust styles

5de3f97

Fix lint

38eb961

Adjust chart styles and options

c1ff1cd

Add coverage expections

2728502

Merge branch 'feature/blockchain-agnostic-new' into 4358-explore-apps…

c3db070

…-statistics

Increase e2e wait time

9f04e0a

Merge branch 'feature/blockchain-agnostic-new' into 4358-explore-apps…

1a79d69

…-statistics

soroushm suggested changes Jul 1, 2022

View reviewed changes

soroushm changed the base branch from feature/blockchain-agnostic-new to feature/blockchain-application July 1, 2022 12:08

Merge branch 'feature/blockchain-application' into 4358-explore-apps-…

96218f5

…statistics

isalga requested a review from eniolam1000752 July 4, 2022 07:38

isalga changed the base branch from feature/blockchain-application to development July 4, 2022 07:53

isalga marked this pull request as ready for review July 4, 2022 07:53

isalga changed the base branch from development to feature/blockchain-application July 4, 2022 08:03

iris salcedo added 4 commits July 4, 2022 10:27

Merge remote-tracking branch 'origin/feature/blockchain-application' …

5fdf0a3

…into 4358-explore-apps-statistics

Add alias

e66b84e

remove old routesMap

94d8692

PR feedback

7d0911d

isalga requested a review from soroushm July 4, 2022 09:01

Update unit test

72c6f5f

soroushm suggested changes Jul 5, 2022

View reviewed changes

Iris Salcedo added 3 commits July 5, 2022 11:18

Merge branch 'feature/blockchain-application' into 4358-explore-apps-…

a0c7460

…statistics

PR feedback

70fb743

Fix test and lint

ba91519

isalga requested a review from soroushm July 5, 2022 10:01

eniolam1000752 suggested changes Jul 5, 2022

View reviewed changes

iris salcedo added 2 commits July 5, 2022 13:24

Add unit tests

847c7d3

Ignore branch coverage

727d36c

isalga requested a review from eniolam1000752 July 5, 2022 11:28

Lint issues

cc2e75a

eniolam1000752 suggested changes Jul 5, 2022

View reviewed changes

soroushm reviewed Jul 5, 2022

View reviewed changes

PR feedback

799a3ab

isalga requested review from soroushm and eniolam1000752 July 5, 2022 12:31

eniolam1000752 approved these changes Jul 5, 2022

View reviewed changes

soroushm approved these changes Jul 5, 2022

View reviewed changes

isalga merged commit 812bbed into feature/blockchain-application Jul 5, 2022

isalga deleted the 4358-explore-apps-statistics branch July 5, 2022 13:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add explore apps statistics component - Closes #4358 #4370

Add explore apps statistics component - Closes #4358 #4370

isalga commented Jun 28, 2022 •

edited

Loading

soroushm Jul 1, 2022

isalga Jul 4, 2022

soroushm Jul 4, 2022

soroushm Jul 1, 2022

isalga Jul 4, 2022

soroushm Jul 4, 2022

soroushm Jul 1, 2022

isalga Jul 4, 2022

soroushm Jul 5, 2022

soroushm Jul 5, 2022

soroushm Jul 5, 2022

soroushm Jul 5, 2022

soroushm Jul 5, 2022

isalga Jul 5, 2022

soroushm Jul 5, 2022

eniolam1000752 commented Jul 5, 2022

eniolam1000752 Jul 5, 2022

eniolam1000752 Jul 5, 2022

eniolam1000752 Jul 5, 2022

eniolam1000752 Jul 5, 2022

eniolam1000752 Jul 5, 2022

soroushm Jul 5, 2022

		@@ -0,0 +1,38 @@
		// eslint-disable-next-line import/prefer-default-export
		export const prepareChartDataAndOptions = (statistics, colorPalette, t) => {

	describe('get blockchain application detail', () => {
	describe('get blockchain application statistics', () => {

Add explore apps statistics component - Closes #4358 #4370

Add explore apps statistics component - Closes #4358 #4370

Conversation

isalga commented Jun 28, 2022 • edited Loading

What was the problem?

How was it solved?

How was it tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eniolam1000752 commented Jul 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isalga commented Jun 28, 2022 •

edited

Loading