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

VL-203 Add simple ui-dashboard #10

Merged
merged 13 commits into from
Feb 15, 2023
Merged

Conversation

datalek
Copy link
Contributor

@datalek datalek commented Feb 10, 2023

Add the dashboard that shows the recorded request-response pair

List of Changes

Add a sub-project
Change package.json to integrate the new project

Motivation and Context

We have to show a simple dashboard that returns the request-response pair handled by the mock.

How Has This Been Tested?

Manual check locally

Types of changes

  • Chore (nothing changes by a user perspective)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@@ -30,7 +30,9 @@
"lint": "eslint src",
"lint:fix": "eslint --fix src",
"prettify": "prettier --write \"./**/*.ts\"",
"start": "npm run compile && node -r dotenv/config dist/main.js dotenv_config_path=.env.default",
"start:client": "cd uidashboard && npm run start",
"start:server": "npm run compile && node -r dotenv/config dist/main.js dotenv_config_path=.env.default",
Copy link

Choose a reason for hiding this comment

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

I suggesto to use npm-run-all so you can choose with the same tool to run tasks sequentially or in parallel. another option is to use task hooks (ie. "prestart:server")

Comment on lines 7 to 11
const uidashboardPath = path.join(
__dirname,
'..',
'..',
'..',
Copy link

Choose a reason for hiding this comment

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

I suggest to have a top level module / config object that exports the root directory path ie.

export rootDir = __dirname;

so you can use path.join(rootDir, "uidashboard", "build")

@@ -0,0 +1,46 @@
# Getting Started with Create React App
Copy link

Choose a reason for hiding this comment

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

I'd use dashes as separator for directory names ui-dashboard


See the section about [deployment](https://facebook.github.io/create-react-app/docs/deployment) for more information.

### `yarn eject`
Copy link

Choose a reason for hiding this comment

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

It looks like we're using npm instead (I see there's a package.json-lock)

@datalek datalek force-pushed the features/add-endpoint-request-response-pair branch from af1ed44 to 9902bc1 Compare February 10, 2023 15:13
@datalek datalek force-pushed the features/add-simple-uidashboard branch from 3e71ff9 to d789d89 Compare February 10, 2023 15:17
@datalek datalek mentioned this pull request Feb 10, 2023
9 tasks
@datalek datalek requested a review from kin0992 February 10, 2023 15:55
@gunzip
Copy link

gunzip commented Feb 11, 2023

If startup time are crucial consider using vite instead of cra -> coryhouse/reactjsconsulting#139

Base automatically changed from features/add-endpoint-request-response-pair to main February 13, 2023 18:16
@datalek datalek marked this pull request as ready for review February 14, 2023 14:33
@@ -23,6 +23,7 @@
"@types/express": "^4.17.16",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need to upload on the origin an updated version of the package-lock.json file, because if I run npm install command, something changes in that file, but it shouldn't change anything


test('renders learn react link', () => {
render(<App />);
const linkElement = screen.getByText(/learn react/i);
Copy link
Contributor

Choose a reason for hiding this comment

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

This test doesn't work, I'd remove it and then later we can think about how to make this kind of test

@datalek datalek requested a review from kin0992 February 15, 2023 07:10
ui-dashboard/package.json Outdated Show resolved Hide resolved
@datalek datalek requested a review from kin0992 February 15, 2023 09:15
Copy link
Contributor

@kin0992 kin0992 left a comment

Choose a reason for hiding this comment

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

🍪

@datalek datalek changed the title Add simple uidashboard VL-203 Add simple ui-dashboard Feb 15, 2023
@datalek datalek merged commit 6c5a55b into main Feb 15, 2023
@datalek datalek deleted the features/add-simple-uidashboard branch February 15, 2023 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants