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

Port SearchPage from Gatsby to NextJS #1583

Merged
merged 1 commit into from
Feb 4, 2021
Merged

Conversation

c3ho
Copy link
Contributor

@c3ho c3ho commented Jan 21, 2021

Issue This PR Addresses

Fixes #1438

Type of Change

  • Bugfix: Change which fixes an issue
  • New Feature: Change which adds functionality
  • Documentation Update: Change which improves documentation
  • UI: Change which improves UI

Description

Recreating PR, previous PR was changed to port the Spinner only.

I had to add null typing to some of the other components to accommodate this PR

This ports the SearchPage from Gatsby to Next

Checklist

  • Quality: This PR builds and passes our npm test and works locally
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
  • Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

@c3ho c3ho changed the title Search page2 Port SearchPage from Gatsby to NextJS Jan 21, 2021
@c3ho c3ho self-assigned this Jan 21, 2021
@manekenpix manekenpix added area: front-end area: nextjs Nextjs related issues labels Jan 21, 2021
@@ -15,7 +15,8 @@
"react": "^16.13.1",
"react-dom": "^16.13.1",
"react-use": "^15.3.8",
"swr": "^0.4.0"
"swr": "^0.4.0",
Copy link
Member

Choose a reason for hiding this comment

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

I think this is in master right now, so you can rebase to get it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I just haven't pushed the newest commits yet and stuff

@humphd
Copy link
Contributor

humphd commented Jan 22, 2021

Are going to get this in today for 1.5?

@c3ho
Copy link
Contributor Author

c3ho commented Jan 22, 2021

@humphd I doubt it, but 1.6 for sure.

@c3ho
Copy link
Contributor Author

c3ho commented Jan 22, 2021

Still failing a few things:

  1. classes.root, classes.title, classes.error
  2. Missing Timeline component
  3. String | null typing in SearchBar, SearchInput and SearchPage

@huyxgit
Copy link

huyxgit commented Jan 27, 2021

@c3ho @humphd should we use onSubmit+click intead of onChange to make request everytime the input changed? or onChange with delay

import { useSWRInfinite } from 'swr';
import { Container, Box, Grid, Typography, ListSubheader } from '@material-ui/core';
import useSiteMetadata from '../../hooks/use-site-metadata';
import Timeline from '../Timeline';
Copy link
Contributor

Choose a reason for hiding this comment

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

Timeline is in now, can we get this rebased so I can test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rebased!

const prepareUrl = (index: number) =>
text
? `${telescopeUrl}/query?text=${encodeURIComponent(text)}&filter=${filter}&page=${index}`
: // Should we return an empty string? or null here?
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would text ever be null? Should we not always have it be a valid string, perhaps '' in the case that the user hasn't done anything yet?

@@ -80,9 +80,9 @@ const useStyles = makeStyles((theme: Theme) =>
);

type searchBarProps = {
text: string;
text: string | null;
Copy link
Contributor

Choose a reason for hiding this comment

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

All these null types: can we not assume that text/string values are either a string of '' (empty string) so nothing breaks, and it will simplify a bunch of code later?

@c3ho
Copy link
Contributor Author

c3ho commented Feb 1, 2021

Some minor styling issues. This mostly works except for when you directly paste a url such as http://localhost:8000/search?text=Josue&filter=author instead of going through standard way of inputting values into the form fields.

Also need to fix styling

src/frontend/next/src/pages/search.tsx Show resolved Hide resolved
event.preventDefault();
setTextParam(text);
setFilterParam(filter);
router.push(`http://localhost:8000/search?text=${text}&filter=${filter}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

localhost:8000?

src/frontend/next/src/pages/search.tsx Show resolved Hide resolved
console.log('testing params');
console.log(router.query);
console.log(textParam);
console.log(filterParam);
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove all this

);
};

export default withRouter(SearchPage);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just use the useRouter() hook?

@humphd
Copy link
Contributor

humphd commented Feb 2, 2021

Adding @rjayroso and @chrispinkney to the reviewers list, since this overlaps with #1546 and #1621.

I'd love to get all of this merged into 1.6. Let's co-ordinate on Slack in the next few days, as all of this is quite close.

import SearchBar from '../components/SearchBar';
import BackToTopButton from '../components/BackToTopButton';

enum FilterParamType {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do it like https://github.com/Seneca-CDOT/telescope/blob/master/src/frontend/next/src/components/SearchResults.tsx#L45, and in a follow-up, we can synchronize all of these to use a common definition of the filter params.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah so something like

type FilterProp = {
  filter: 'post' | 'author';
};

and to type the hooks I use

const [filter, setFilter] = useState<FilterProp['filter']>('post');

src/frontend/next/src/pages/search.tsx Show resolved Hide resolved
humphd
humphd previously approved these changes Feb 4, 2021
import SearchBar from '../components/SearchBar';
import BackToTopButton from '../components/BackToTopButton';

// Required to type filter or else TS lint will yell at you
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be removed, it's standard with TS to need this.

folder changes

changes

added use-query-params package
Copy link
Contributor

@chrispinkney chrispinkney left a comment

Choose a reason for hiding this comment

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

Loved the comments, really helping me understand a lot of this code which is otherwise pretty foreign to me. 😄

@chrispinkney chrispinkney requested a review from humphd February 4, 2021 19:01
@humphd
Copy link
Contributor

humphd commented Feb 4, 2021

Thanks for doing this @c3ho, land ho!

@c3ho c3ho merged commit 8ebd0e9 into Seneca-CDOT:master Feb 4, 2021
@c3ho
Copy link
Contributor Author

c3ho commented Feb 4, 2021

The buttons are going to look bad, but I guess that's a separate issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: front-end area: nextjs Nextjs related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port SearchPage from Gatsby to NextJS
6 participants