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

[next] Fixes #1546: Ported SearchResults to NextJS and fixed styling on search error #1625

Merged
merged 1 commit into from Feb 2, 2021

Conversation

rjayroso
Copy link
Contributor

@rjayroso rjayroso commented Jan 29, 2021

Issue This PR Addresses

Fixes #1546 (Port SearchResults to NextJS)
Fixes #1458 (Update error styling for failed search)
Closes #1512 (The original 1458 issue was worked on by @sonechca and his resulting PR's work was essentially merged into this one and updated)

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
  • Next.JS Port

Description

This PR ports SearchResults component from Gatsby to NextJS. This PR also includes an update on the search error styling.

Notable changes are the removal of the loading variable from const { data, size, loading, setSize, error } = useSWRInfinite(//...)
loading is now declared as const loading = !data && !error;

className={classes.root} attribute was removed from <Box className={classes.root} boxShadow={2} marginTop={10}> since classes.root doesn't exist (Typescript complained whereas in the original JSX file this was not an issue).

CSS styling additions of errorBackground, errorTitle, and errorMessage were added to update the error styling.

How to Test

Import the SearchResults component into a NextJS page (I used the index.tsx to test):
import SearchResults from '../components/SearchResults'

Now create a SearchResults component and pass in some data:
e.g. it might look like this <SearchResults text={"Search for me"} filter={"I am a filter"}/>

Save the code and then refresh the page, you should see something similar to this:
image

To check the responsiveness, inspect the page (right-click the page -> inspect) and toggle device toolbar. For resulting example, see below:

Styling was taken from PR #1512 and updated to be responsive, see below:

Before After
image image

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)

@rjayroso rjayroso self-assigned this Jan 29, 2021
@rjayroso rjayroso changed the title Ported SearchResults to NextJS and fixed styling on search error [next] Fixes #1546: Ported SearchResults to NextJS and fixed styling on search error Jan 29, 2021
HyperTHD
HyperTHD previously approved these changes Jan 29, 2021
Copy link
Contributor

@HyperTHD HyperTHD left a comment

Choose a reason for hiding this comment

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

Other than the typo, looks good otherwise. I'm a little uneasy with the way it looks on mobile, but that's probably because the banner component hasn't been added yet.

src/frontend/next/src/components/SearchResults.tsx Outdated Show resolved Hide resolved
@HyperTHD HyperTHD added this to In progress in Nextjs via automation Jan 29, 2021
@HyperTHD HyperTHD added this to the 1.6 Release milestone Jan 29, 2021
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.

Getting:

Unhandled Runtime Error

TypeError: page.map is not a function

from Timeline.tsx after checking out this PR. Any ideas? Master runs just fine for me until I test this in pages/index.tsx

firefox_2021-01-29_18-54-50

@rjayroso
Copy link
Contributor Author

@chrispinkney wow I've never seen it say anything about that before, I don't think I touched anything in Timeline.tsx but if I had to guess it has to do something with maybe how I set up my environment(?). I set it up so that my backend comes from the dev server instead of running docker locally.

@rjayroso
Copy link
Contributor Author

Can I see how you imported the component and implemented it in your pages/index.tsx?

Nextjs automation moved this from In progress to Review in progress Jan 30, 2021
@PedroFonsecaDEV
Copy link
Contributor

PedroFonsecaDEV commented Jan 30, 2021

Getting:

Unhandled Runtime Error

TypeError: page.map is not a function

from Timeline.tsx after checking out this PR. Any ideas? Master runs just fine for me until I test this in pages/index.tsx

firefox_2021-01-29_18-54-50

Let's investigate this bug in a different thread because I don't think it is related to this PR.

@HyperTHD
Copy link
Contributor

HyperTHD commented Feb 1, 2021

Getting:

Unhandled Runtime Error

TypeError: page.map is not a function

from Timeline.tsx after checking out this PR. Any ideas? Master runs just fine for me until I test this in pages/index.tsx
firefox_2021-01-29_18-54-50

Let's investigate this bug in a different thread because I don't think it is related to this PR.

The SearchPage won't be able to land with this issue still intact, we must solve it here

@humphd
Copy link
Contributor

humphd commented Feb 1, 2021

How can I reproduce this pages.map error? I'd like to look into it.

@manekenpix
Copy link
Member

manekenpix commented Feb 1, 2021

I think this is a filter issue. Currently, our filter only accepts post or author. Any other value would cause the page.map error. I'm testing using author or post as filters and it works wonderfully.

@manekenpix
Copy link
Member

Peek 2021-01-31 21-42

@humphd
Copy link
Contributor

humphd commented Feb 1, 2021

Interesting. In TS we can capture that with type Filter = 'post' | 'author'

@rjayroso
Copy link
Contributor Author

rjayroso commented Feb 1, 2021

Interesting. In TS we can capture that with type Filter = 'post' | 'author'

Would this statement be in the SearchResults.tsx component or in the Search page (before we pass the filter prop)?

@humphd
Copy link
Contributor

humphd commented Feb 1, 2021

@rjayroso anything that relies on that typing would need it, just like you might use string in multiple places for a String type.

@rjayroso
Copy link
Contributor Author

rjayroso commented Feb 1, 2021

I added type Filter = 'post' | 'author';, and have gotten the same results as @manekenpix. As long as filter is either post or author, it no longer gives us that page.map error.
I tested it further with the change you suggested but when I wrapped data in another array (pages={data} to pages={[data]}), it gave me this:
image


type SearchResultProps = {
text: string;
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.

Get rid of 43 and do this inline, since we don't do anything else with Filter:

filter: 'post' | 'author';

Copy link
Contributor

Choose a reason for hiding this comment

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

One thing to consider: we might want to define Filter like you did on 43 in one file and export it, so we can use it again in another. I'm not sure who the owner of the type should be, maybe the root search page? Regardless, that can happen in an other PR. cc @c3ho, who I think will have to deal with this.

@humphd
Copy link
Contributor

humphd commented Feb 1, 2021

Let's rebase this too

* Removed unused component Theme
* Fixed typo in the error message
* Added type Filter
* Changes type Filter to be defined inline
Nextjs automation moved this from Review in progress to Reviewer approved Feb 2, 2021
@rjayroso rjayroso merged commit 1a17008 into Seneca-CDOT:master Feb 2, 2021
Nextjs automation moved this from Reviewer approved to Done Feb 2, 2021
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 type: enhancement New feature or request
Projects
No open projects
Nextjs
  
Done
Development

Successfully merging this pull request may close these issues.

[next] Port SearchResults to NextJS [next] Update 500 error UI styling for failed search
6 participants