-
Notifications
You must be signed in to change notification settings - Fork 30
UI Feat: Migrating the landing page to design token system #108
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
base: main
Are you sure you want to change the base?
Conversation
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a95f3528a1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ); | ||
| } | ||
|
|
||
| {/*Sub compentent class*/} |
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.
Remove top-level JSX comment that breaks parsing
The standalone {/*Sub compentent class*/} is JSX syntax but it appears at the top level (outside any JSX return), which causes the JS parser to throw an “Unexpected token” error during build. This will fail any environment that tries to compile this file, so replace it with a normal JS comment (e.g., // ...) or remove it.
Useful? React with 👍 / 👎.
du-dhartley
left a comment
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 PR is looking good, but has a couple of items to address. We should also remove the comments in LandingPage.js as they're not necessary.
| @@ -1,3 +1,4 @@ | |||
| @import "./styles/tokens.css"; | |||
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.
Doesn't global.css already import this? Is it required here?
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.
Yes I believe it does, I will remove it with the fix.
| import React from "react"; | ||
| import {Link} from "react-router-dom"; | ||
|
|
||
| export default function LandingPage() { |
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.
With the removal of the onSignInClick and onAboutClick props, the parent component that calls them should also be updated to ensure that we don't break anything
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.
Ok no worries, I'm currently working on the fix to the conflicts.
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.
Can you please advise as to how you still have a .js file? All these files were tranisitioned into .jsx approximately a month ago
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.
Hey sorry, I've been working between my laptop and main PC when I go away for work. I assume my laptop never properly updated the files. I'm currently working to correct all this now.
|
Caleb there seem to be some conflicts with the landing page, could you see if they can get resolved |
| import React from "react"; | ||
| import {Link} from "react-router-dom"; | ||
|
|
||
| export default function LandingPage() { |
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.
Can you please advise as to how you still have a .js file? All these files were tranisitioned into .jsx approximately a month ago
Pushing UI feature. This involves implementing tokens fully as well as swapping the current landing page to move towards design tokens. The goal is to create a fully universal design relying on tokens rather then hardcoded variables. This creates a centralized design that can be changed from the one location rather then each individual page.
Key changes:
Migrating Landing page from hardcoded values to design tokens.
Fully integrating Design tokens into the system to allow the rest of the UI (e.g. Evidence Dashboard) to be migrated to design tokens.