-
Notifications
You must be signed in to change notification settings - Fork 2
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
Default IQ Dashboard to dark theme, update URLs, and implement translation #291
Conversation
Run & review this pull request in StackBlitz Codeflow. |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@Aliiiu please remember to review this pull request |
@@ -1,23 +1,24 @@ | |||
'use client' |
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 think we can extract the client-side interaction into a separate component. so it will be server(parent) - client(child)
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.
Thought of making this changes, but didn't want to cause conflicts since you already are working on refactoring in another pr. This PR didn't make changes to this.
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 am only working on refactoring specific API routes
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.
Oh okay. We'll do this alongside other refactoring changes in a new pr then
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.
the PR should be about changing default bg update urls and translation. everything else is just delaying the merge.
code reviews should also be about the specs and if its an important needed refactoring, you open a new issue, merge this PR and work in that new issue 👍🏻
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.
Okay, great! Will ensure focus is narrowed down to specs
import Link from '@/components/elements/LinkElements/Link' | ||
import TokenData from '@/components/dashboard/TokenData' | ||
import { DashboardGraphData } from '@/components/dashboard/GraphDetails' | ||
import { fetchMarketData } from '@/utils/fetch-market-data' | ||
import { useTranslations } from 'next-intl' | ||
import { IQLogo } from '@/components/iq-logo' | ||
|
||
const Home: NextPage = () => { | ||
const { marketData } = fetchMarketData() |
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.
why is this not a hook? Looking at the code structure and implementation, this appears to be a React hook rather than a utility function.
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 for a different task, not for refactoring. This PR didn't make changes to this either. It only updates IQ default theme to dark mode, updates URLs to https://iq.iqai.com/ and implements translation.
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.
Alright, noted
} | ||
|
||
const RootLayout = ({ children }: { children: React.ReactNode }) => { | ||
export default async function RootLayout({ |
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 the second RootLayout component the other is inside app-[locale]-layout. Also why is it that one has import 'regenerator-runtime/runtime' and the other does not
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.
Nice catch 👏 Was an oversight. Will rename the second to LocaleLayout. For the regenerator-runtime/runtime, wasn't a change made in this PR.
return ( | ||
<html lang="en"> | ||
<html lang={locale} suppressHydrationWarning> | ||
<head> |
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.
why do we still need the head tag after exporting generateMetadata
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 didn't make changes to that. It only translated the exported generateMetadata
@@ -8,20 +8,20 @@ interface SEOHeaderProps { | |||
|
|||
const SEOHeader = ({ router }: SEOHeaderProps) => ( |
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.
Are we still using this 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.
Just confirmed it's redundant. Again, this PR didn't make changes to that. We might need to handle removing all redundant files in the refactoring pr
import { ChakraProvider } from '@chakra-ui/react' | ||
import chakraTheme from '@/theme' | ||
import ColorMode from '@/components/chakra/ColorMode' | ||
import { Link } from '@/i18n/routing' |
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.
why are we using this Link 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.
It enables us to work with pathnames like /about, while i18n aspects like language prefixes are handled behind the scenes. https://next-intl.dev/docs/getting-started/app-router/with-i18n-routing
@@ -166,8 +169,7 @@ const VotingPage = () => { | |||
> | |||
<EmptyState /> | |||
<Text maxW="80" color="tooltipColor" fontWeight="medium"> | |||
There are no active votings at the moment, Votes in progress will appear | |||
here as they happen. | |||
{t('emptyState')} | |||
</Text> | |||
</Flex> | |||
) |
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.
The votingItem has missing translations
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.
Nice catch 👏 Will update
import React from 'react' | ||
import BinanceIcon from '../icons/binance' | ||
import { OneInch } from '../icons/1inch' | ||
import { Upbit } from '../icons/Upbit' | ||
import ArrowIcon from '../icons/arrow' | ||
import { Fraxswap } from '../icons/fraxswap' | ||
import { useTranslations } from 'next-intl' | ||
import { Link } from '@/i18n/routing' |
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.
Why are we using this 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.
The 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.
yes, the exchanges are external link
@@ -6,7 +6,7 @@ import { Button } from '@/components/ui/button' | |||
import { RiArrowDownSFill, RiCheckFill, RiFileCopyLine } from 'react-icons/ri' | |||
import { getShortenAddress } from '@/lib/helpers/getShortenAddress' | |||
import { useState } from 'react' | |||
import Link from 'next/link' | |||
import { Link } from '@/i18n/routing' |
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.
please revisit this import also
|
||
type ChakraLinkAndNextProps = ChakraLinkProps & LinkProps | ||
|
||
export const LinkWrapper = ({ href, children }: ChakraLinkAndNextProps) => { | ||
return ( | ||
<NextLink href={href} passHref legacyBehavior> | ||
<Link href={href} passHref legacyBehavior> |
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.
do we really need the passHref and legacyBehavior. Let's also try to separate this component to be used for external and internal component
@Softdev1 this PR is huge, ensure you also test it before merging, i dont want people having issues staking or other errors. Ensure you check all pages work as expected. Also once merged, perhaps send it to Andy to review korean translations |
Okay sir |
Code Climate has analyzed commit 040e6ca and detected 4 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
Default IQ Dashboard to dark theme, update URLs, and implement translation
This PR implements translation for all pages, updates URLs to https://iq.iqai.com/ and updates IQ default theme to dark mode
Linked issues
closes EveripediaNetwork/issues#3699