Skip to content
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

feat: Handle Mobile and Desktop Specific routes #394

Merged
merged 14 commits into from
Apr 17, 2024

Conversation

Xavier-Charles
Copy link
Contributor

@Xavier-Charles Xavier-Charles commented Apr 10, 2024

In this PR View routes like /b/aave are resolved to search Routes using the board-slug map in the boards config.

Also routes that contain /superfeed are rerouted to / on desktop instead of 404

@Xavier-Charles Xavier-Charles marked this pull request as ready for review April 10, 2024 12:38
Copy link
Contributor

@v-almonacid v-almonacid left a comment

Choose a reason for hiding this comment

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

As mentioned yesterday we won't follow this approach.

thegraph: ["the-graph"],
trading: ["market", "trading-video", "economic-calendar"],
verasity: ["verasity", "verasity-source"],
vitalikbuterin: ["vitalki-buterin"],
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

@@ -40,6 +40,8 @@ export interface IRoute {
component: typeof ErrorPage | typeof PreloaderPage | typeof DashboardPage;
exact?: boolean;
isFullsize?: boolean;
redirectTo?: string;
type: "regular" | "redirect";
Copy link
Contributor

Choose a reason for hiding this comment

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

note that this definition is inconsistent to the mobile route definition, and using a union type for type doesn't allow for runtime casting

key={route.path}
path={route.path}
exact={route.exact}
to={route.redirectTo ?? EDesktopRoutePaths.Base}
Copy link
Contributor

Choose a reason for hiding this comment

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

if the route is of type redirect, the redirectTo field shouldn't be optional (relates to my other comment on type definition).

const { IS_DEV } = CONFIG;
const { IS_DEV, BOARDS } = CONFIG;

const BoardRoutesHandler = (
Copy link
Contributor

Choose a reason for hiding this comment

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

A function name which is not a react FC should never start with an uppercase. I keep telling you this, Charles. You are no longer a junior dev, you should pay more attention to coding style.

packages/frontend/src/MobileApp.tsx Show resolved Hide resolved
Copy link
Contributor

@v-almonacid v-almonacid left a comment

Choose a reason for hiding this comment

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

@v-almonacid v-almonacid marked this pull request as draft April 17, 2024 13:23
@v-almonacid v-almonacid marked this pull request as ready for review April 17, 2024 13:24
Copy link
Member

@elcharitas elcharitas left a comment

Choose a reason for hiding this comment

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

This looks good to me @Xavier-Charles

Thanks

@v-almonacid v-almonacid marked this pull request as draft April 17, 2024 14:42
@v-almonacid v-almonacid marked this pull request as ready for review April 17, 2024 16:41
@v-almonacid v-almonacid merged commit 79da491 into dev Apr 17, 2024
4 checks passed
@v-almonacid v-almonacid deleted the feat/ALPHA-4966-Alphaday-Mobile-Redirect branch April 17, 2024 17:38
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.

3 participants