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

784 faq page #812

Merged
merged 31 commits into from Sep 24, 2020
Merged

784 faq page #812

merged 31 commits into from Sep 24, 2020

Conversation

ghost
Copy link

@ghost ghost commented Sep 22, 2020

Fixes #784

  • Initial Page:

Screen Shot 2020-09-22 at 3 48 46 PM

  • While Searching:

Screen Shot 2020-09-22 at 3 49 47 PM

  • After Search:

Screen Shot 2020-09-22 at 3 51 16 PM

  • Up to date with dev branch
  • Branch name follows guidelines
  • All PR Status checks are successful
  • Peer reviewed and approved

Any questions? See the getting started guide

@ghost ghost requested a review from adamkendis September 23, 2020 18:31
@ghost
Copy link
Author

ghost commented Sep 23, 2020

@adamkendis running into two logged errors currently that I haven't been able to fix. They are working as expected, but still log errors.

line 29 of FaqHeader, getting "TypeError: Cannot read property 'target' of undefined but it does seem to read the value and insert it into the input

line 69 of FaqHeader, getting TypeError: Cannot read property 'appendChild' of null, it is appending each line that matches to the autocomplete div

Copy link
Member

@adamkendis adamkendis left a comment

Choose a reason for hiding this comment

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

Hey Hannah, this is really excellent. Works great, completely keyboard accessible. One small change to the position of the link in the header. Regarding the errors, I'm somewhat sure this is what's happening.

  const autocompleteDiv = document.querySelector('.autocomplete-items');
  const searchBox = document.querySelector('#autocompete-search');

The first time the FaqHeader function is called, autocompleteDiv and searchBox are initialized before the function returns the html. So, .autocomplete-items and #autocompete-search don't exist at the time the variables are initialized, but only the first time the component mounts. You can see this by console logging autocompleteDiv and searchBox right after they're declared and/or inside the handleChange function.

I think you can eliminate the errors by using the useRef hook instead of document.querySelector because the hook operates inside the component's lifecycle.

Great job!

@@ -35,6 +35,11 @@ const Header = () => {

<div id="navbar" className="navbar-menu">
<div className="navbar-end">
<div className="navbar-item">
Copy link
Member

Choose a reason for hiding this comment

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

Could you move the help center link to the right side of the header?

Copy link
Author

Choose a reason for hiding this comment

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

Move to the right! Combined with the responsive navbar you pushed up, it now pops up on the left next to toggle when page scales down FYI

Comment on lines 13 to 14
const autocompleteDiv = document.querySelector('.autocomplete-items');
const searchBox = document.querySelector('#autocompete-search');
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure querySelector is causing the two errors you mentioned. Try useRef instead of document.querySelector.

https://reactjs.org/docs/hooks-reference.html#useref

Copy link
Author

Choose a reason for hiding this comment

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

That worked perfectly, took care of the errors!

import FaqRequestTypes from '@assets/faq/311-explain-request-types.png';
import FaqCompareCouncils from '@assets/faq/311-compare-councils.png';

export const FAQS = [
Copy link
Member

Choose a reason for hiding this comment

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

I like the way you structured this.

@ghost
Copy link
Author

ghost commented Sep 24, 2020

@adamkendis Fixed merge conflict from other changes that were recently made to the navbar. I'm failing lint for that section of the nav that is commented "es lint disabled", do we need to disable the lint check on that section locally?

@adamkendis adamkendis self-requested a review September 24, 2020 18:39
Copy link
Member

@adamkendis adamkendis left a comment

Choose a reason for hiding this comment

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

Looks good!

@adamkendis adamkendis merged commit f56b62a into hackforla:dev Sep 24, 2020
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.

FAQ Page
1 participant