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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new package for Spotlight Client #228

Merged
merged 20 commits into from
Oct 14, 2020
Merged

Add new package for Spotlight Client #228

merged 20 commits into from
Oct 14, 2020

Conversation

macfarlandian
Copy link
Collaborator

@macfarlandian macfarlandian commented Oct 14, 2020

Description of the change

More configuration than code, but this adds a third package where we will start adding features for the new Spotlight application. Of note:

  • The new package was created with Create React App and Typescript. I borrowed some ESLint and TS configurations from the COVID site, since those mostly worked well.
  • Type checks are part of the lint script. They run in CI as well as in the pre-commit hook.
  • We are now sending Jest coverage reports to Coveralls 馃帀
    • This involved a fair amount of wrestling with the Github CI configuration, due to some gaps in documentation as well as the added complexity of having multiple packages in this repo. In the end I settled on a new single-workflow setup; this does not affect our ability to run independent jobs in parallel, but it does affect our ability to only run certain jobs when there are changes to the package directory they belong to. In practice this isn't that important anyway because we have to run all the tests to get a proper composite coverage figure, and the builds are still pretty fast.

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Configuration change (adjusts configuration to achieve some end related to functionality, development, performance, or security)

Related issues

Closes #218

Checklists

Development

These boxes should be checked by the submitter prior to merging:

  • Manual testing against realistic data has been performed locally

Code review

These boxes should be checked by reviewers prior to merging:

  • This pull request has a descriptive title and information useful to a reviewer
  • This pull request has been moved out of a Draft state, has no "Work In Progress" label, and has assigned reviewers
  • Potential security implications or infrastructural changes have been considered, if relevant

Copy link
Member

@jessex jessex left a comment

Choose a reason for hiding this comment

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

Love it 馃檶

.gitignore Outdated
@@ -38,3 +38,6 @@ yarn-error.log*

# Package files
package-lock.json

# Typescript build artifacts
*.tsbuildinfo
Copy link
Member

Choose a reason for hiding this comment

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

Nit: newline at the end

@@ -0,0 +1,13 @@
import React from "react";
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Let's make sure we've got license text at the top of each file in the app. We didn't do that in Covid-19 Dashboard but that was simply because I didn't bother to add that extra bit of friction during that time. But we should do it for every file we generate going forward that supports comments.

import App from "./App";

ReactDOM.render(
<React.StrictMode>
Copy link

Choose a reason for hiding this comment

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

cool! didn't know about StrictMode before 馃憤

@macfarlandian macfarlandian merged commit 35b089c into master Oct 14, 2020
@macfarlandian macfarlandian deleted the 218-new-package branch October 14, 2020 18:40
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.

Create a new React application for spotlight-client
3 participants