Refactor source [ch124583] #165

Clebal · 2021-01-22T09:35:37Z

Refactored data & source structure in template-sample-app.
Refactored hygen layer to fit new structure.
Added hygen source to generate sources.
Refactored data & source structure in template-skeleton.
Added documentation.

From this CH ticket.

Proposal: #164.

Refactored hygen layer to fit new structure. Added hygen source to generate sources.

shortcut-integration · 2021-01-22T09:35:39Z

This pull request has been linked to Clubhouse Story #124583: Refactor source.

vercel · 2021-01-22T09:35:42Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/carto-frontend/cra-template-carto/d4fy2lfjq
✅ Preview: https://cra-template-carto-git-feature-ch124583refactor-source.carto-frontend.vercel.app

hygen/_templates/layer/new/inject_view_useEffect.ejs.t

hygen/_templates/layer/new/prompt.js

aaranadev · 2021-01-22T11:08:57Z

hygen/_templates/layer/new/prompt.js

+    if (answers.view.includes('views/')) {
+      const selectedViewInitialPath = answers.view.split('(')[1].replace(')', '');
+      answers.view_path = viewFiles.find(viewFile => {
+        return viewFile.path === selectedViewInitialPath.replace('views', VIEWS_DIR)
+      }).path
+    } else {
+      answers.view_path = VIEWS_DIR
    }


Improved:

Suggested change

if (answers.view.includes('views/')) {

const selectedViewInitialPath = answers.view.split('(')[1].replace(')', '');

answers.view_path = viewFiles.find(viewFile => {

return viewFile.path === selectedViewInitialPath.replace('views', VIEWS_DIR)

}).path

} else {

answers.view_path = VIEWS_DIR

}

answers.view_path = VIEWS_DIR

if (answers.view.includes('views/')) {

const selectedViewInitialPath = answers.view.split('(')[1].replace(')', '');

answers.view_path = viewFiles.find(viewFile => {

return viewFile.path === selectedViewInitialPath.replace('views', VIEWS_DIR)

}).path

}

Chapó 👌

aaranadev · 2021-01-22T11:52:09Z

hygen/_templates/promptUtils.js

+  return files
+    .reduce((a, f) => a.concat(f), [])
+    .map(file => (typeof file === 'string' ? { path: file.split('src/')[file.split('src/').length - 1], name: file.split('/')[file.split('/').length - 1] } : file))
+    .filter(({ name }) => /[A-Z]/.test(name[0]) && name.includes('.js'));


The map maybe be a reducer and the filter function is included.

@albertoarana fixed.

aaranadev · 2021-01-22T11:55:24Z

hygen/_templates/source/new/prompt.js

+  const fileName = [answers.name[0].toUpperCase(), ...answers.name.slice(1, answers.name.length)].join('')
+  const doesSourceAlreadyExists = await doesFileExists(['src', 'data', 'sources', `${fileName}Source.js`]);
+  if (doesSourceAlreadyExists) throw new Error(`The source ${answers.name} already exists.`);


If source already exists, the code is executed but nothing happens because hygen has the skip flag if that source exists.

hygen/_templates/source/new/source.ejs.t

Co-authored-by: Alberto Arana <aarana@carto.com>

…created.

VictorVelarde · 2021-01-25T16:22:07Z

template-sample-app/template/src/components/layers/StoresLayer.js

@@ -28,7 +28,7 @@ export default function StoresLayer() {

  if (storesLayer && source) {
    return new CartoSQLLayer({
-      id: 'storesPointLayer',
+      id: LayerStyle.id,


I think id should be extracted from LayerStyle object, as it is conceptually not included inside style props, what do you think?

An exported const at the top could be useful, as with sources

@VictorVelarde that change is contemplated in the next PR where @Clebal modifies the generator Layer

VictorVelarde · 2021-01-25T16:26:20Z

template-sample-app/template/src/components/views/Kpi.js


 export default function Kpi() {
  const dispatch = useDispatch();

+  const LAYER_ID = 'kpiLayer';


After the good refactor you have done on Source, removing the const here and exposing it from the 'source' file, I think we should probably do the same for layers: exporting a const with the 'id' from each Layer file, and consuming it here with kpiLayer.id. That way, there is just one place for the LAYER_ID, and that is the layer itself, which IMO it makes sense...

what do you think?

It's being done in #167.

VictorVelarde · 2021-01-25T16:26:56Z

template-sample-app/template/src/components/views/Kpi.js

-        column='name'
-        operationColumn='revenue'
+        dataSource={kpiSource.id}
+        column={KPI_SOURCE_COLUMNS.NAME}


this looks nice!

VictorVelarde · 2021-01-25T16:28:48Z

template-sample-app/template/src/components/views/datasets/Datasets.js

@@ -55,6 +55,8 @@ function Datasets() {
    };
  }, [credentials, dispatch]);

+  // Auto import useEffect
+
  return (
    <Grid


We're gonna remove this file (all related to that tab), but I'll wait till this gets merged, to avoid any conflict

aaranadev · 2021-01-26T08:24:16Z

@VictorVelarde I like suggestion. We are going to change it

aaranadev · 2021-01-26T08:55:51Z

Done. I have needed to change the file names to camelCase.

alasarr · 2021-01-26T20:42:02Z

template-sample-app/template/src/data/sources/kpiSource.js

@@ -0,0 +1,19 @@
+export const KPI_SOURCE_ID = 'kpiSource';


I'd simplify this to only export the default.

It adds a data structure that it's not required and it adds complexity in several parts of the code.

KPI_SOURCE_ID is available as default.id
KPI_SOURCE_COLUMNS is like have two ways to define the source: at the SQL and the const.

It's a key concept for new users and we need to simplify this as much as possible.

If someone wants to complicate more to have a more robust architecture is fine, but not for all the users

…r template

VictorVelarde

Fixed some conflicts after merging client-side PR

alasarr and others added 7 commits November 20, 2020 19:46

Template description metadata (#119)

feebf18

Change diagram (#120)

403c0f3

Refactored data & source structure in template-sample-app.

8f1148a

Refactored hygen layer to fit new structure. Added hygen source to generate sources.

Refactored data & source structure in template-skeleton.

95183e3

Minor changes to fit code style.

f6fe6b4

Added documentation and minor changes in code due to documentation.

2ddbf34

Minor format fixes in documentation.

3a96d7c

Clebal requested review from VictorVelarde and aaranadev January 22, 2021 09:35

vercel bot had a problem deploying to Preview January 22, 2021 09:35 Failure

Adapt StoresList to the new source structure.

089a92b

vercel bot deployed to Preview January 22, 2021 09:47 View deployment

Clebal mentioned this pull request Jan 22, 2021

Vercel seems to be broken #166

Closed

aaranadev requested changes Jan 22, 2021

View reviewed changes

Update hygen/_templates/layer/new/prompt.js

0c30ed3

Co-authored-by: Alberto Arana <aarana@carto.com>

vercel bot deployed to Preview January 22, 2021 12:04 View deployment

Remove file exists check in source generator

f6b4153

vercel bot deployed to Preview January 22, 2021 12:18 View deployment

Clebal added 2 commits January 22, 2021 13:48

Improved getFiles helper function definition.

6545a68

Added throw error if there's no source or view when a layer is being …

9045af4

…created.

vercel bot deployed to Preview January 22, 2021 12:50 View deployment

aaranadev approved these changes Jan 22, 2021

View reviewed changes

Clebal mentioned this pull request Jan 22, 2021

Refactor hygen layer [ch131112] #167

Merged

VictorVelarde reviewed Jan 25, 2021

View reviewed changes

VictorVelarde changed the base branch from develop to master January 25, 2021 16:46

aaranadev added 4 commits January 26, 2021 09:48

wip

8dbc1d0

rename files

2949a6b

wip

3128171

rename files

b746265

vercel bot deployed to Preview January 26, 2021 08:55 View deployment

fixed hygen

fa92f99

vercel bot deployed to Preview January 26, 2021 09:39 View deployment

alasarr suggested changes Jan 26, 2021

View reviewed changes

Simplify source structure.

2c9494f

vercel bot deployed to Preview January 27, 2021 10:20 View deployment

Added modifications to changelog.

2b33516

vercel bot deployed to Preview January 27, 2021 11:49 View deployment

Minor changes in hygen to fit new file name convention for sources.

41f9e71

vercel bot deployed to Preview January 27, 2021 12:02 View deployment

Make proper changes in skeleton template.

d461efd

vercel bot deployed to Preview January 27, 2021 14:43 View deployment

chore: update changelog with latest changes in master

868ba7d

vercel bot deployed to Preview January 27, 2021 19:05 View deployment

refactor: use the new hook for source filtering (client-side) in laye…

bfe19a3

…r template

vercel bot deployed to Preview January 27, 2021 19:47 View deployment

VictorVelarde added 2 commits January 27, 2021 20:56

refactor: add latest changes in kpi page at master

e0f6f72

refactor: remove duplicated action in stores + some cleaning

0ea951d

vercel bot deployed to Preview January 27, 2021 20:02 View deployment

Merge branch 'master' into feature/ch124583/refactor-source

7c94157

vercel bot deployed to Preview January 27, 2021 20:30 View deployment

VictorVelarde approved these changes Jan 27, 2021

View reviewed changes

VictorVelarde merged commit 8e37ab1 into master Jan 27, 2021

VictorVelarde deleted the feature/ch124583/refactor-source branch January 27, 2021 20:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor source [ch124583] #165

Refactor source [ch124583] #165

Clebal commented Jan 22, 2021 •

edited

shortcut-integration bot commented Jan 22, 2021

vercel bot commented Jan 22, 2021 •

edited

aaranadev Jan 22, 2021

Clebal Jan 22, 2021

aaranadev Jan 22, 2021

Clebal Jan 22, 2021

aaranadev Jan 22, 2021

Clebal Jan 22, 2021

VictorVelarde Jan 25, 2021

VictorVelarde Jan 25, 2021

aaranadev Jan 25, 2021

VictorVelarde Jan 25, 2021

Clebal Jan 25, 2021

VictorVelarde Jan 25, 2021

VictorVelarde Jan 25, 2021

aaranadev commented Jan 26, 2021

aaranadev commented Jan 26, 2021

alasarr Jan 26, 2021 •

edited

VictorVelarde left a comment

Refactor source [ch124583] #165

Refactor source [ch124583] #165

Conversation

Clebal commented Jan 22, 2021 • edited

shortcut-integration bot commented Jan 22, 2021

vercel bot commented Jan 22, 2021 • edited

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

aaranadev commented Jan 26, 2021

aaranadev commented Jan 26, 2021

alasarr Jan 26, 2021 • edited

Choose a reason for hiding this comment

VictorVelarde left a comment

Choose a reason for hiding this comment

Clebal commented Jan 22, 2021 •

edited

vercel bot commented Jan 22, 2021 •

edited

alasarr Jan 26, 2021 •

edited