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

#8 Linting Auto-Fixes #58

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@
"build": "react-scripts build",
"test": "react-scripts test",
"eject": "react-scripts eject",
"lint": "eslint ."
"lint": "eslint .",
"lint:fix": "eslint . --fix"
},
"eslintConfig": {
"extends": "react-app"
Expand Down
4 changes: 2 additions & 2 deletions src/components/AddGroup.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export const AddGroup = withRouter((props) => {
}}
>
<div className="form-group">
<label for="inputCommunityName">New community name</label>
<label htmlFor="inputCommunityName">New community name</label>
<input type="text" className="form-control" id="inputCommunityName"
placeholder="Enter community name"
ref={node => {
Expand All @@ -45,7 +45,7 @@ export const AddGroup = withRouter((props) => {
<small className="form-text text-danger">* required</small>
</div>
<div className="form-group">
<label for="inputCommunityDescription">Description</label>
<label htmlFor="inputCommunityDescription">Description</label>
<input type="text" className="form-control" id="inputCommunityName"
placeholder="Enter community description"
ref={node => {
Expand Down
6 changes: 3 additions & 3 deletions src/components/AddResponse.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ import React, { useState } from 'react';
import { withRouter } from "react-router"
import { useMutation } from '@apollo/react-hooks';
import { errorHandler } from '../services/graphql/errorHandler'
import { CREATE_POST } from '../services/graphql/queries';
import { THREAD } from '../services/graphql/queries';
import { CREATE_POST , THREAD } from '../services/graphql/queries';


export const AddResponse = withRouter((props) => {
const [showResponseForm, setShowResponseForm] = useState(false)
Expand Down Expand Up @@ -50,7 +50,7 @@ export const AddResponse = withRouter((props) => {
onClick={() => setShowResponseForm(!showResponseForm)}>share your response</button>
}

let body = <div className="mx-0 mb-3">
const body = <div className="mx-0 mb-3">
{responseForm}
</div>

Expand Down
2 changes: 1 addition & 1 deletion src/components/ContactForm.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ export const ContactForm = (props) => {
return (
<div className="bg-white rounded shadow-sm ">
<div className="m-3">
<iframe src="https://docs.google.com/forms/d/e/1FAIpQLSeVV5hF5Lf9uVW5Utd0kI_QYmkzlOexun4vpd-0iHj7ExMDVQ/viewform?embedded=true" className="container-fluid" width="100%" height="1013" frameborder="0" marginheight="0" marginwidth="0">Loading…</iframe>
<iframe src="https://docs.google.com/forms/d/e/1FAIpQLSeVV5hF5Lf9uVW5Utd0kI_QYmkzlOexun4vpd-0iHj7ExMDVQ/viewform?embedded=true" className="container-fluid" width="100%" height="1013" frameBorder="0" marginHeight="0" marginWidth="0">Loading…</iframe>
</div>
</div>
);
Expand Down
10 changes: 5 additions & 5 deletions src/components/GroupPreview.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,24 +4,24 @@ import { Thread } from './Thread';

export const GroupPreview = withRouter((props) => {
let activeConversations
let threadTitles = []
const threadTitles = []
if (props.threads.length > 0) {
for (let i=0; i < props.threads.length && i < 3; i++) {
threadTitles.push(
<li class="list-group-item">{props.threads[i].title}</li>
<li className="list-group-item">{props.threads[i].title}</li>
)
}
activeConversations = <div>
<div class="card-header text-center">
<div className="card-header text-center">
Active Conversations
</div>
<ul class="list-group list-group-flush">
<ul className="list-group list-group-flush">
{threadTitles}
</ul>
</div>
}

let url = "/community/" + props.groupId
const url = `/community/${ props.groupId}`
Copy link
Contributor

Choose a reason for hiding this comment

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

For this one, I would update the spaces in the string to something like this:

Suggested change
const url = `/community/${ props.groupId}`
const url = `/community/${props.groupId}`

Makes it a tad more readable that way.

return (
<div className="card col-sm m-3">
<div className="card-body">
Expand Down
8 changes: 4 additions & 4 deletions src/components/Groups.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ export const Groups = withRouter((props) => {
if (error) return <p>Error :(</p>;

if (data.groups.length > 0) {
let groupCards = []
const groupCards = []
data.groups.forEach((group, i) =>{
groupCards.push(<GroupPreview
key={group.id}
Expand All @@ -28,7 +28,7 @@ export const Groups = withRouter((props) => {
threads={group.threads} />)
if((i+1) % 3 == 0){
groupCards.push(
<div class="w-100"></div>
<div className="w-100" />
)
}
});
Expand All @@ -39,11 +39,11 @@ export const Groups = withRouter((props) => {
</div>
</div>
);
} else {
}
return (
<div>
<h1>welcome.</h1>
</div>
)
}

});
2 changes: 1 addition & 1 deletion src/components/Index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ export const Index = withRouter((props) => {
<Header />
<p className="lead text-center">Hawthorn is a place for communities;</p>
<p className="text-center">a platform with commitment to social justice, premised on mutual support and owned by you, the community</p>
<div class="alert alert-warning" role="alert">
<div className="alert alert-warning" role="alert">
<h1 className="display-4 text-center">Hawthorn's Transition to Small Private Groups</h1>
<p className="text-center"><u>Updated January 20, 2020</u></p>
<p className="text-center">
Expand Down
28 changes: 14 additions & 14 deletions src/components/Navigation.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,25 +2,25 @@ import React from 'react'

export const Navigation = (props) => {
return (
<nav class="navbar navbar-expand-md navbar-light fixed-top bg-light">
<a class="navbar-brand" href="/">Hawthorn</a>
<button class="navbar-toggler" type="button" data-toggle="collapse" data-target="#primaryNavigation" aria-controls="primaryNavigation" aria-expanded="false" aria-label="Toggle navigation">
<span class="navbar-toggler-icon"></span>
<nav className="navbar navbar-expand-md navbar-light fixed-top bg-light">
<a className="navbar-brand" href="/">Hawthorn</a>
<button className="navbar-toggler" type="button" data-toggle="collapse" data-target="#primaryNavigation" aria-controls="primaryNavigation" aria-expanded="false" aria-label="Toggle navigation">
<span className="navbar-toggler-icon" />
</button>

<div class="collapse navbar-collapse" id="primaryNavigation">
<ul class="navbar-nav mr-auto">
<li class="nav-item">
<a class="nav-link" href="/communities">Browse Communities</a>
<div className="collapse navbar-collapse" id="primaryNavigation">
<ul className="navbar-nav mr-auto">
<li className="nav-item">
<a className="nav-link" href="/communities">Browse Communities</a>
</li>
<li class="nav-item">
<a class="nav-link" href="/about">About Hawthorn</a>
<li className="nav-item">
<a className="nav-link" href="/about">About Hawthorn</a>
</li>
<li class="nav-item">
<a class="nav-link" href="/code-of-conduct">Code of Conduct</a>
<li className="nav-item">
<a className="nav-link" href="/code-of-conduct">Code of Conduct</a>
</li>
<li class="nav-item">
<a class="nav-link" href="/contact-us">Contact Us</a>
<li className="nav-item">
<a className="nav-link" href="/contact-us">Contact Us</a>
</li>
<li className="nav-item dropdown">
<a className="nav-link dropdown-toggle" data-toggle="dropdown" href="#" role="button" aria-haspopup="true" aria-expanded="false">Account</a>
Expand Down
6 changes: 3 additions & 3 deletions src/components/Register.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ const Register = withRouter((props) => {
>
<div className="form-row">
<div className="col-md-4 mb-3">
<label for="inputUsername">Username</label>
<label htmlFor="inputUsername">Username</label>
<div className="input-group">
<div className="input-group-prepend">
<span className="input-group-text" id="inputGroupPrepend">@</span>
Expand All @@ -52,7 +52,7 @@ const Register = withRouter((props) => {
</div>
</div>
<div className="col-md-4 mb-3">
<label for="inputEmail">Email</label>
<label htmlFor="inputEmail">Email</label>
<div className="input-group">
<input type="email" className="form-control" id="inputEmail" aria-describedby="emailHelp" placeholder="Enter email" required ref={node => {
email = node;
Expand All @@ -62,7 +62,7 @@ const Register = withRouter((props) => {
</div>
</div>
<div className="form-group">
<label for="inputEmail">Password</label>
<label htmlFor="inputEmail">Password</label>
<input type="password" className="form-control" id="inputEmail" placeholder="Password" required ref={node => {
password = node;
}}/>
Expand Down
8 changes: 4 additions & 4 deletions src/components/Routes.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import React from 'react';
import { withRouter } from "react-router"
import { useQuery } from '@apollo/react-hooks';
import { Switch, Route } from "react-router-dom";
import { errorHandler } from '../services/graphql/errorHandler'
import { FUSIONAUTH_CONFIG } from '../services/graphql/queries'
import { Switch, Route } from "react-router-dom";
import { About } from './About';
import Account from './Account';
import { CodeOfConduct } from './CodeOfConduct';
Expand All @@ -20,20 +20,20 @@ import { Thread } from './Thread';

export const Routes = withRouter((props) => {
// Retrieve authorization config from the server so we can build auth URIs
var { loading, error, data } = useQuery(FUSIONAUTH_CONFIG,
const { loading, error, data } = useQuery(FUSIONAUTH_CONFIG,
{
onError(error) {
errorHandler(error, props.history)
}
})
if (loading) {
return <p>Loading</p>
} else if (error) {
} if (error) {
return <p>An unexpected error occurred, please come back later</p>
}
if (loading) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this if (loading)... code duplicated? If so, may be worth cleaning up while you're making changes here.

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, this pattern will most likely change.

return <p>Loading</p>
} else if (error) {
} if (error) {
return <p>An unexpected error occurred, please come back later</p>
}
const configuration = {
Expand Down
8 changes: 4 additions & 4 deletions src/components/Thread.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ export const Thread = withRouter((props) => {
<h3 className="text-center">
<button
type="button"
className={"btn btn-link btn-lg"}
onClick={() => {props.history.push('/community/' + data.thread.group.id)}}>
className="btn btn-link btn-lg"
onClick={() => {props.history.push(`/community/${ data.thread.group.id}`)}}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same suggestion here in terms of readability of the string:

Suggested change
onClick={() => {props.history.push(`/community/${ data.thread.group.id}`)}}>
onClick={() => {props.history.push(`/community/${data.thread.group.id}`)}}>

Copy link
Contributor

Choose a reason for hiding this comment

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

We will we using an internal <Button /> component for this in the future

&larr; back to '{data.thread.group.name}'
</button>
</h3>
Expand All @@ -59,11 +59,11 @@ export const Thread = withRouter((props) => {
</div>
</div>
)
} else {
}
return (
<div className="border-bottom border-gray m-3">
<p>Thread not found</p>
</div>
)
}

})
6 changes: 3 additions & 3 deletions src/components/ThreadPreview.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { withRouter } from "react-router"
import { Post } from './Post';

export const ThreadPreview = withRouter((props) => {
let posts = <div className="ml-0">
const posts = <div className="ml-0">
{/* Limit post content for preview */}
{props.posts.slice(0, 2).map((post) =>
<Post
Expand All @@ -17,8 +17,8 @@ export const ThreadPreview = withRouter((props) => {
<h4>{props.title}</h4>
<button
type="button"
className={"btn btn-outline-primary"}
onClick={() => {props.history.push('/thread/' + props.threadId)}}>
className="btn btn-outline-primary"
onClick={() => {props.history.push(`/thread/${ props.threadId}`)}}>
Copy link
Contributor

Choose a reason for hiding this comment

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

And here:

Suggested change
onClick={() => {props.history.push(`/thread/${ props.threadId}`)}}>
onClick={() => {props.history.push(`/thread/${props.threadId}`)}}>

Copy link
Contributor

Choose a reason for hiding this comment

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

We will we using an internal component for this in the future

join conversation
</button>
{posts}
Expand Down
4 changes: 2 additions & 2 deletions src/components/Threads.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@ export const Threads = withRouter((props) => {
</ul>
</div>
);
} else {
}
return (
<p className="m-3">This is a new community. Start the first conversation if you have something on your mind.</p>
)
}

})
2 changes: 1 addition & 1 deletion src/configuration/configuration.js
Original file line number Diff line number Diff line change
@@ -1 +1 @@
export const configuration = require ('./configuration.json');
export default require ('./configuration.json');
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 sync up about module syntax and exports/imports and figure out how we want to do this one. There may be more to this than simply switching it to be default instead of named.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would like to talk module export/import syntax as well. I don't understand the tradeoffs of the different options and what is best practice.

2 changes: 1 addition & 1 deletion src/index.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import React from 'react';
import ReactDOM from 'react-dom';
import { BrowserRouter } from "react-router-dom";
import App from './App';
import * as serviceWorker from './serviceWorker';
import { BrowserRouter } from "react-router-dom";

ReactDOM.render((
<BrowserRouter>
Expand Down
70 changes: 35 additions & 35 deletions src/serviceWorker.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,40 +20,6 @@ const isLocalhost = Boolean(
)
);

export function register(config) {
if (process.env.NODE_ENV === 'production' && 'serviceWorker' in navigator) {
// The URL constructor is available in all browsers that support SW.
const publicUrl = new URL(process.env.PUBLIC_URL, window.location.href);
if (publicUrl.origin !== window.location.origin) {
// Our service worker won't work if PUBLIC_URL is on a different origin
// from what our page is served on. This might happen if a CDN is used to
// serve assets; see https://github.com/facebook/create-react-app/issues/2374
return;
}

window.addEventListener('load', () => {
const swUrl = `${process.env.PUBLIC_URL}/service-worker.js`;

if (isLocalhost) {
// This is running on localhost. Let's check if a service worker still exists or not.
checkValidServiceWorker(swUrl, config);

// Add some additional logging to localhost, pointing developers to the
// service worker/PWA documentation.
navigator.serviceWorker.ready.then(() => {
console.log(
'This web app is being served cache-first by a service ' +
'worker. To learn more, visit https://bit.ly/CRA-PWA'
);
});
} else {
// Is not localhost. Just register service worker
registerValidSW(swUrl, config);
}
});
}
}

function registerValidSW(swUrl, config) {
navigator.serviceWorker
.register(swUrl)
Expand Down Expand Up @@ -126,10 +92,44 @@ function checkValidServiceWorker(swUrl, config) {
});
}

export function register(config) {
if (process.env.NODE_ENV === 'production' && 'serviceWorker' in navigator) {
// The URL constructor is available in all browsers that support SW.
const publicUrl = new URL(process.env.PUBLIC_URL, window.location.href);
if (publicUrl.origin !== window.location.origin) {
// Our service worker won't work if PUBLIC_URL is on a different origin
// from what our page is served on. This might happen if a CDN is used to
// serve assets; see https://github.com/facebook/create-react-app/issues/2374
return;
}

window.addEventListener('load', () => {
const swUrl = `${process.env.PUBLIC_URL}/service-worker.js`;

if (isLocalhost) {
// This is running on localhost. Let's check if a service worker still exists or not.
checkValidServiceWorker(swUrl, config);

// Add some additional logging to localhost, pointing developers to the
// service worker/PWA documentation.
navigator.serviceWorker.ready.then(() => {
console.log(
'This web app is being served cache-first by a service ' +
'worker. To learn more, visit https://bit.ly/CRA-PWA'
);
});
} else {
// Is not localhost. Just register service worker
registerValidSW(swUrl, config);
}
});
}
}

export function unregister() {
if ('serviceWorker' in navigator) {
navigator.serviceWorker.ready.then(registration => {
registration.unregister();
});
}
}
}
2 changes: 1 addition & 1 deletion src/services/graphql/errorHandler.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
export const errorHandler = (error, history) => {
export default (error, history) => {
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 talk about this one too - way may need to update this (or consuming code).

Copy link
Contributor

@zacksabbath zacksabbath Feb 15, 2020

Choose a reason for hiding this comment

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

Would also like to discuss this. I think we should think about a different paradigm concerning error handling, I see a lot of duplication. A wrapper function / custom hook may be a good solution.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed. Error handling needs some love.

if (error.graphQLErrors) {
error.graphQLErrors.forEach(graphQLError => {
console.log('graphQLError', graphQLError)
Expand Down