Skip to content

Conversation

@bruceplai
Copy link
Member

Closes #909

  • Update Google Analytics measurement ID

  • Update root route to point to homepage

  • Simplify Layout

  • Update breadcrumbs to use root route

  • Update spec files to use root route

  • Remove references to old landing page

return (
<nav className={classes.nav}>
<Link to='/home'>
<Link to='/'>

Choose a reason for hiding this comment

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

Identical blocks of code found in 2 locations. Consider refactoring.

return (
<nav className={classes.nav}>
<Link to='/home'>
<Link to='/'>

Choose a reason for hiding this comment

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

Identical blocks of code found in 2 locations. Consider refactoring.

Copy link
Contributor

@bhaggya bhaggya left a comment

Choose a reason for hiding this comment

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

On cypress/fixtures/faqs.json file you have changed the a href to "http://civictechindex.org\" But on FAQ page on What is Civic Tech Index? On clicking
on Civic Tech Index hyperlink , it still goes to "http://civictechindex.org/home" page

@bruceplai
Copy link
Member Author

On cypress/fixtures/faqs.json file you have changed the a href to "[http://civictechindex.org](http://civictechindex.org%5C)" But on FAQ page on What is Civic Tech Index? On clicking on Civic Tech Index hyperlink , it still goes to "http://civictechindex.org/home" page

@bhaggya faqs.json is for testing. If you go the FAQ page, then it currently calls the production backend API. That data returned by the API still has the old homepage URL. You can verify this by fetching the raw JSON in your browser with the following URL: https://api.civictechindex.org/api/faqs/

@bhaggya
Copy link
Contributor

bhaggya commented Oct 10, 2021

On cypress/fixtures/faqs.json file you have changed the a href to "http://civictechindex.org" But on FAQ page on What is Civic Tech Index? On clicking on Civic Tech Index hyperlink , it still goes to "http://civictechindex.org/home" page

@bhaggya faqs.json is for testing. If you go the FAQ page, then it currently calls the production backend API. That data returned by the API still has the old homepage URL. You can verify this by fetching the raw JSON in your browser with the following URL: https://api.civictechindex.org/api/faqs/

I didnt know that ,the API needs to be changed

@bruceplai
Copy link
Member Author

@ladissi we will most likely need to update some data in the production db that are still referring to the /home route since that should no longer exist.

@mealthebear
Copy link
Member

This is purely code style preferences: I noticed several files are inconsistent with ordering of keys in their breadcrumb links.
(Ex.)

{ href: '/', name: 'Home' } vs { name: 'Home', href: '/' }  

It could be nice to change them to one standardized style, it seems like more files use the "name, href" format as of right now. If it's not that critical, we can forgo this for a later issue.

@bruceplai
Copy link
Member Author

This is purely code style preferences: I noticed several files are inconsistent with ordering of keys in their breadcrumb links. (Ex.)

{ href: '/', name: 'Home' } vs { name: 'Home', href: '/' }  

It could be nice to change them to one standardized style, it seems like more files use the "name, href" format as of right now. If it's not that critical, we can forgo this for a later issue.

Good catch. I cleaned up the breadcrumb objects so href comes first

maxskewes
maxskewes previously approved these changes Oct 15, 2021
Copy link
Member

@maxskewes maxskewes left a comment

Choose a reason for hiding this comment

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

Really solid and thorough.

mealthebear
mealthebear previously approved these changes Oct 15, 2021
Copy link
Member

@mealthebear mealthebear 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 to me

Comment on lines +22 to 24
<Link to='/'>
<img className={classes.logo} src='/images/cti-logo.svg' alt='civic logo' />
</Link>
Copy link
Member

@mealthebear mealthebear Oct 18, 2021

Choose a reason for hiding this comment

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

I could be wrong about this; but React Context API could be useful here to get rid of the Codeclimate duplicate code error and share this unit of code with the HeaderSmall.js & HeaderLarge.js files. Definitely something more appropriate for an entirely separate issue, though. Just figured I would throw this out on the radar.

</Hidden>
</Container>
</Box>
<Box className='containerWhite'>

Choose a reason for hiding this comment

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

Identical blocks of code found in 3 locations. Consider refactoring.

@bruceplai bruceplai removed the request for review from christianagu January 21, 2022 05:19
@bruceplai bruceplai marked this pull request as draft February 25, 2022 01:03
@bruceplai bruceplai force-pushed the update-landing-909 branch from 5ec115b to 8137e0e Compare March 19, 2022 17:59
* Update Google Analytics measurement ID

* Update root route to point to homepage

* Simplify Layout

* Update breadcrumbs to use root route

* Update spec files to use root route

* Remove references to old landing page
@bruceplai bruceplai force-pushed the update-landing-909 branch from e374360 to cd9d020 Compare April 8, 2022 03:00
@qlty-cloud-legacy
Copy link

Code Climate has analyzed commit 5914d40 and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 3

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 80.9% (0.0% change).

View more on Code Climate.

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.

Google Analytics update URL and remove /home

5 participants