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.

Sign up for GitHub

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

Jump to bottom

explorer-client: initialise client app #421

Merged
merged 1 commit into from
Feb 17, 2022
Merged

explorer-client: initialise client app #421

merged 1 commit into from
Feb 17, 2022

Conversation

pchrysochoidis
Copy link
Contributor

@pchrysochoidis pchrysochoidis commented Feb 11, 2022 •
edited
Loading

  • project init with create-react-app with typescript template
  • added suport for scss
  • updated readme with some info for development
  • added linting/formating support with prettier, eslint and stylelint
  • using .env to disable reporting web-vitals in prod mode and console.log them in dev
  • disabled react-scripts from opening the browser with env variable in .env.development (can be overridden in a local env file)

resolves #420

@sblackshear sblackshear changed the title exlporer-client: initialise client app explorer-client: initialise client app Feb 13, 2022
apburnie
apburnie approved these changes Feb 15, 2022
View reviewed changes
Copy link
Contributor

@apburnie apburnie left a comment

Choose a reason for hiding this comment

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

This is perfect as a starter CRA

stella3d
stella3d suggested changes Feb 15, 2022
View reviewed changes
Copy link
Contributor

@stella3d stella3d left a comment

Choose a reason for hiding this comment

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

I think we should remove some stuff that isn't used - "web vitals" & commented out code.

We should also remove the React logo - I think we have some in-progress logos for the network we can use.

explorer/client/src/react-app-env.d.ts
@@ -0,0 +1 @@
/// <reference types="react-scripts" />
Copy link
Contributor

Choose a reason for hiding this comment

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

let's remove this file if it's commented out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a special comment for ts to include some types (not sure what exactly was auto generated so I assumed it includes something that is necessary) https://www.typescriptlang.org/docs/handbook/triple-slash-directives.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's useful actually. It defines types for various file types like .module.scss, images etc

Copy link
Contributor

Choose a reason for hiding this comment

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

Good spot. Although strange, this type declaration file is needed to allow TypeScript to work with files that are not typed. If you delete it you'll notice a lot of complaints regarding no corresponding type declarations.

Copy link
Contributor

Choose a reason for hiding this comment

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

ahh, ok. strange but 👍

explorer/client/src/utils/reportWebVitals.ts
@@ -0,0 +1,37 @@
import type { ReportHandler } from 'web-vitals';
Copy link
Contributor

Choose a reason for hiding this comment

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

can we just remove all the web vitals stuff ? doesn't seem necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since it's under config flag I don't see any reason not to keep it. When this goes to prod probably we will use them to monitor the performance (send the metrics to a service or something) - I am ok to remove it if you insist as well

Copy link
Contributor

Choose a reason for hiding this comment

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

I've put a comment in the Slack channel to get some thoughts on whether we are likely to be using Google Analytics.

@pchrysochoidis
Copy link
Contributor Author

@stella3d I added this issue #458 for the icons. I can add them if we have them already or merge this and add them later

@pchrysochoidis
explorer-client: initialise client app
7751f7b 
* project init with `create-react-app` with typescript template
* added suport for scss
* updated readme with some info for development
* added linting/formating support with prettier, eslint and stylelint
* using `.env` to disable reporting `web-vitals` in prod mode and `console.log` them in dev
* disabled `react-scripts` from opening the browser with env variable in `.env.development` (can be overridden in a local env file)
* added classnames lib
@pchrysochoidis pchrysochoidis force-pushed the pc-420-init-explorer-client branch from 8987f85 to 7751f7b Compare February 16, 2022 14:01
@pchrysochoidis
Copy link
Contributor Author

Updated the PR to

  • fix a typo in the commit message (exlporer -> explorer)
  • fix lint scripts that didn't run on mac/zsh
  • added classnames lib
  • added another script start:dev to run concurrently start and prettier:fix:watch

@stella3d
Copy link
Contributor

logos will come end of week from partners, analytics and thus web vitals are an open question, so i think this is good to merge for now.

@pchrysochoidis pchrysochoidis merged commit 72de8f8 into main Feb 17, 2022
@pchrysochoidis pchrysochoidis deleted the pc-420-init-explorer-client branch February 17, 2022 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stella3d stella3d stella3d approved these changes

@apburnie apburnie Awaiting requested review from apburnie

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[explorer-client] initialise the FE app
3 participants
@pchrysochoidis @stella3d @apburnie