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
Sc 12333/ensign footer #120
Conversation
This pull request has been linked to Shortcut Story #12333: Ensign Landing Footer component. |
<li> | ||
<a href="https://rotational.io/ensign">Ensign</a> | ||
</li> | ||
{/* <li> |
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.
This and the SDK links will be hidden until they are available. A follow up story will be created in Shortcut.
function Footer() { | ||
return ( | ||
<footer className="bg-footer bg-cover bg-no-repeat"> | ||
<div className="pt-64 2xl:pt-80"> |
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.
As the screen width increases, content in the footer goes above the background image. 2xl:pt-80
is a small hack to prevent this from happening. While updating the website's footer, I'll look into a better way of handing this.
@@ -0,0 +1,26 @@ | |||
import * as Sentry from '@sentry/react'; |
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.
This is the current Sentry configuration for rotational.app. I wanted to make sure we didn't forget to add this, but this may need to be updated.
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.
would you mind moving this to application/config
instead ?
@@ -0,0 +1,13 @@ | |||
{ |
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.
This was added so that Lingui will work correctly.
<meta property="og:type" content="website"> | ||
<meta property="og:title" content="Ensign by Rotational Labs"> | ||
<meta property="og:description" content="Ensign by Rotational Labs"> | ||
<meta property="og:image" content=""> |
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'll create a follow up story to add images to the meta tags. Also, Twitter is listed separately because last I checked their site has stopped recognizing og tags.
@@ -1,6 +1,11 @@ | |||
{ | |||
"name": "beacon-app", | |||
"version": "0.2.0", | |||
"description": "User UI for Ensign.", |
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.
This was in the setup for the ensign-user-ui, so this was added for consistency.
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.
Kudos 👍 .
Few minor changes that need to be done,
Also, would you mind adding this component to the storybook?
@@ -0,0 +1,26 @@ | |||
import * as Sentry from '@sentry/react'; |
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.
would you mind moving this to application/config
instead ?
const dsn = process.env.REACT_APP_SENTRY_DSN; | ||
const environment = process.env.REACT_APP_SENTRY_ENVIRONMENT | ||
? process.env.REACT_APP_SENTRY_ENVIRONMENT | ||
: process.env.NODE_ENV; | ||
|
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.
we are going to avoid using process.env
directly, what do you think if you define those environments in application/config/appConfig
and consume them everywhere it's needed?
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've made the update. I didn't see the config file before.
); | ||
} | ||
|
||
export default Footer; |
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.
export default Footer; | |
export default React.memo(Footer); |
Would you mind making this component a pure component?
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 got an error with this change so, I imported memo and did the following export default memo(LandingFooter);
. I also updated the file name to be a little more specific since this footer won't be used once a user registers or logs in.
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.
LGTM
Scope of changes
Adds Footer component to Beacon UI. Also, updates the React app setup by adding the following:
Some additional clean up was done by deleting the ensign-user-ui, since beacon-app is replacing it.
Note: It was decided to not focus on mobile responsiveness before the MVP release. Responsive styling will be updated in the future so that the footer appears better on smaller screens.
Fixes SC-12333
Type of change
Acceptance criteria
Author checklist
Reviewer(s) checklist