Skip to content
This repository has been archived by the owner on Dec 9, 2020. It is now read-only.

Display temp & RH in graph for each station #127

Merged
merged 13 commits into from
May 28, 2020
Merged

Conversation

Kyubinhan
Copy link
Collaborator

No description provided.

@Kyubinhan Kyubinhan changed the title Story/temp rh graph/244 Display temp & RH in graph for each station May 28, 2020
Copy link
Contributor

@Sybrand Sybrand left a comment

Choose a reason for hiding this comment

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

A few questions, and one concern in the Dockerfile, otherwise looks good. Let's chat about the Dockerfile.

// Theme documentation: https://material-ui.com/customization/palette/
// Theme demo: https://material.io/resources/color/#!/?view.left=1&view.right=1&primary.color=003365&secondary.color=FBC02D
// Do not export this directly! theme should be accessed within makeStyles & withStyles. Use ErrorMessage.tsx as a reference
export const theme = createMuiTheme({
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment says not to export?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I had to make it exportable so that I can use the same theme in Storybook .storybook/decorators/ThemeDecorator.tsx. I will change the wording to Do not export this directly for styling!

@@ -7,7 +7,7 @@ WORKDIR /app
COPY package*.json ./

## Install only the packages defined in the package-lock.json (faster than the normal npm install)
RUN npm set progress=false && npm ci --production
RUN npm set progress=false && npm ci --no-cache
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we rather want?

Suggested change
RUN npm set progress=false && npm ci --no-cache
RUN npm set progress=false && npm ci --no-cache --production

"typescript": "^3.7.5"
},
"scripts": {
"start": "react-scripts start",
"start": "CI=true react-scripts start",
Copy link
Contributor

Choose a reason for hiding this comment

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

What does the CI=true do?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not exactly sure what it does but it fixes the issue where it fails to start in the docker container.
facebook/create-react-app#8688 (comment)

@Sybrand Sybrand self-requested a review May 28, 2020 21:47
@sonarcloud
Copy link

sonarcloud bot commented May 28, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
2.5% 2.5% Duplication

@Kyubinhan Kyubinhan merged commit 8dc6165 into master May 28, 2020
@Kyubinhan Kyubinhan deleted the story/temp-rh-graph/244 branch May 28, 2020 23:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants