-
Notifications
You must be signed in to change notification settings - Fork 30
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
Update landing page (#909) #932
base: main
Are you sure you want to change the base?
Conversation
@@ -19,7 +19,7 @@ const HeaderSmall = () => { | |||
|
|||
return ( | |||
<nav className={classes.nav}> | |||
<Link to='/home'> | |||
<Link to='/'> |
There was a problem hiding this comment.
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.
@@ -12,7 +12,7 @@ const HeaderLarge = () => { | |||
|
|||
return ( | |||
<nav className={classes.nav}> | |||
<Link to='/home'> | |||
<Link to='/'> |
There was a problem hiding this comment.
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.
There was a problem hiding this 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
@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 |
@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. |
This is purely code style preferences: I noticed several files are inconsistent with ordering of keys in their breadcrumb links.
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 |
0197ff2
to
070458a
Compare
There was a problem hiding this 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.
There was a problem hiding this 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
<Link to='/'> | ||
<img className={classes.logo} src='/images/cti-logo.svg' alt='civic logo' /> | ||
</Link> |
There was a problem hiding this comment.
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.
e6ef1b6
ccb6429
to
e6ef1b6
Compare
e6ef1b6
to
bf96261
Compare
</Hidden> | ||
</Container> | ||
</Box> | ||
<Box className='containerWhite'> |
There was a problem hiding this comment.
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.
2e523b3
to
5ce8cd1
Compare
5505a8a
to
f175561
Compare
5ec115b
to
8137e0e
Compare
* 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
e374360
to
cd9d020
Compare
Code Climate has analyzed commit 5914d40 and detected 3 issues on this pull request. Here's the issue category breakdown:
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. |
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