feat: set styles for new routing #3494

andre-code · 2025-01-20T14:10:00Z

PR to handle atyles for new routing

Tests will be handled in #3503.
Functionalities such as creating a new v2 project or v2 group will be addressed in #3501.

Summary of Changes

Included in the Update:

Landing Page: Now loads with styles v2
Renku 1.0 Compatibility:
* /v1 now loads the Renku 1.0 dashboard when the user is authenticated.
- Routes under /v1/* load the corresponding Renku 1.0 pages:
  - /v1/search
  - /v1/help
  - /v1/notifications
  - /v1/secrets
Projects and Datasets:
- Routes under /projects/* now load Renku 1.0 project pages.
- Similarly, /datasets/* routes load the appropriate content from Renku 1.0.

Pending Fixes to solve in (#3501):

Style Guide: The style guide is currently broken and will be deprecated in this context.
Route Adjustments for /v2:
- /secrets, /search, and /help should load the content currently served under /v2/secrets, /v2/search, and /v2/help.
- /notifications should load what is in /v1/notifications??? pending to decide since /v2/notifications doesn’t exist
Project and Group Creation Redirects:
- Issues with creating projects or groups in v2 need to be resolved.
- The new redirect from /v2 to /r should address these inconsistencies.

/deploy

RenkuBot · 2025-01-20T14:10:49Z

You can access the deployment of this PR at https://renku-ci-ui-3494.dev.renku.ch

by default styles v2, styles v1 apply only for /v1, /projects/*, /datasets/*

leafty

Some first comments below 👇

Also, can you update the PR description with the routing changes? And also what has been broken by these changes?

leafty · 2025-01-22T12:18:46Z

client/src/routing/routes.constants.ts

+  v1: {
+    root: "v1/*",
+    search: "search",
+    help: "help",


Where are notifications, styleGuide, secrets?

I have included in the last commit

leafty · 2025-01-22T12:19:23Z

client/src/notifications/Notifications.present.jsx

@@ -323,7 +325,7 @@ class NotificationsMenuList extends Component {

    return (
      <Fragment>
-        <Link to="/notifications">
+        <Link to={Url.get(ABSOLUTE_ROUTES.v1.notifications)}>


Do not use Url.get() with ABSOLUTE_ROUTES.

Suggested change

<Link to={Url.get(ABSOLUTE_ROUTES.v1.notifications)}>

<Link to={ABSOLUTE_ROUTES.v1.notifications}>

leafty · 2025-01-22T12:20:35Z

client/src/utils/helpers/HelperFunctions.js

+export function isRenkuLegacy(pathname) {
+  return (
+    pathname.startsWith("/v1") ||
+    pathname.startsWith("/projects") ||
+    pathname.startsWith("/datasets")
+  );
+}
+


It would be better to add this to a TypeScript file. It is OK to have HelperFunctionsV2.ts.

leafty · 2025-01-22T12:24:30Z

client/src/landing/NavBar.jsx

+        <LoggedInNavBar
+          model={props.model}
+          notifications={props.notifications}
+          params={props.params}
+        />


Note: this should probably be a V1 navbar which is LoggedInNavBar or AnonymousNavBar.

leafty · 2025-01-22T12:26:45Z

client/src/index.jsx

@@ -141,16 +140,36 @@ function FeatureFlagHandler() {
 export function StyleHandler() {
  return (
    <Switch>
-      <CompatRoute path="/v2">
+      <Route path="/projects">


Are we not using the RELATIVE_ROUTES here?

I have included in the last commit

leafty · 2025-01-22T12:30:24Z

client/src/components/navbar/NavBarItems.tsx

@@ -212,7 +213,7 @@ export function RenkuToolbarHelpMenu({ firstItem }: RenkuToolbarHelpMenuProps) {
        aria-labelledby="help-menu"
      >
        <DropdownItem className="p-0">
-          <Link className="dropdown-item" to="/help">
+          <Link className="dropdown-item" to={Url.get(ABSOLUTE_ROUTES.v1.help)}>


Suggested change

<Link className="dropdown-item" to={Url.get(ABSOLUTE_ROUTES.v1.help)}>

<Link className="dropdown-item" to={ABSOLUTE_ROUTES.v1.help}>

leafty · 2025-01-22T12:30:38Z

client/src/components/navbar/LoggedInNavBar.tsx

@@ -98,7 +99,7 @@ export default function LoggedInNavBar({
                className="nav-item col-12 col-sm-4 col-lg-auto pe-lg-4"
              >
                <RenkuNavLink
-                  to={Url.get(Url.pages.landing)}
+                  to={Url.get(ABSOLUTE_ROUTES.v1.root)}


Suggested change

to={Url.get(ABSOLUTE_ROUTES.v1.root)}

to={ABSOLUTE_ROUTES.v1.root}

leafty · 2025-01-22T12:30:46Z

client/src/components/navbar/LoggedInNavBar.tsx

@@ -85,7 +86,7 @@ export default function LoggedInNavBar({
            >
              <NavItem className="nav-item col-12 col-sm-4 col-lg-auto pe-lg-4">
                <RenkuNavLink
-                  to={Url.get(Url.pages.search)}
+                  to={Url.get(ABSOLUTE_ROUTES.v1.search)}


Suggested change

to={Url.get(ABSOLUTE_ROUTES.v1.search)}

to={ABSOLUTE_ROUTES.v1.search}

leafty · 2025-01-22T12:31:13Z

client/src/App.jsx

-        location={location}
-        {...props}
-      />
+      <AppContext.Provider value={appContext}>


Any specific reason to move AppContext.Provider? Also, if you do move it to include more of the App, then it would be nice to remove the params prop drilling.

the NavbarV2 uses parameters from the appContext, so to make those values available, I rewrote the other component to also retrieve the values from the appContext

leafty · 2025-01-22T12:33:19Z

client/src/features/rootV1/RootV1.tsx

+            }
+          />
+          <Route
+            path="help/*"


Use RELATIVE_ROUTES here.

- Fix handling of absolute and relative routes. - Remove prop drilling for params, modal, and notifications in App.jsx. - Prevent redirection to / when the user is unauthenticated in /v1. - Display the anonymous navigation bar on /v1, /projects, and /datasets when the user is not authenticated.

andre-code · 2025-01-23T14:43:25Z

Thanks, @leafty , for the review! I’ve applied your suggestions.

leafty

One comment about the style handler, otherwise this looks good. 👏

leafty · 2025-01-24T08:53:57Z

client/src/index.jsx

@@ -141,16 +141,26 @@ function FeatureFlagHandler() {
 export function StyleHandler() {
  return (
    <Switch>
-      <CompatRoute path="/v2">
+      <Route path={RELATIVE_ROUTES.projects}>


Should the style handler use the isRenkuLegacy() function instead? With doing the routing here and having the isRenkuLegacy() function, we have a risk of not having the style corresponding to what isRenkuLegacy() says.

ciyer · 2025-01-24T13:13:42Z

This is really great, thanks for making these changes, you've already done a bunch of work from my build! :)

andre-code temporarily deployed to renku-ci-ui-3494 January 20, 2025 14:10 — with GitHub Actions Inactive

andre-code force-pushed the andrea/styles-launch-build branch from 8abfc99 to 1b9be89 Compare January 21, 2025 14:49

andre-code had a problem deploying to renku-ci-ui-3494 January 21, 2025 14:49 — with GitHub Actions Failure

andre-code temporarily deployed to renku-ci-ui-3494 January 21, 2025 15:17 — with GitHub Actions Inactive

feat: set styles for new routing

4802e5d

by default styles v2, styles v1 apply only for /v1, /projects/*, /datasets/*

andre-code force-pushed the andrea/styles-launch-build branch from 1b9be89 to 4802e5d Compare January 22, 2025 09:10

andre-code had a problem deploying to renku-ci-ui-3494 January 22, 2025 09:11 — with GitHub Actions Error

andre-code changed the base branch from main to launch-build January 22, 2025 09:16

andre-code had a problem deploying to renku-ci-ui-3494 January 22, 2025 09:16 — with GitHub Actions Failure

andre-code temporarily deployed to renku-ci-ui-3494 January 22, 2025 09:34 — with GitHub Actions Inactive

andre-code marked this pull request as ready for review January 22, 2025 10:05

andre-code requested a review from a team as a code owner January 22, 2025 10:05

andre-code temporarily deployed to renku-ci-ui-3494 January 22, 2025 10:06 — with GitHub Actions Inactive

andre-code temporarily deployed to renku-ci-ui-3494 January 22, 2025 10:20 — with GitHub Actions Inactive

leafty self-assigned this Jan 22, 2025

leafty reviewed Jan 22, 2025

View reviewed changes

andre-code temporarily deployed to renku-ci-ui-3494 January 23, 2025 08:53 — with GitHub Actions Inactive

andre-code temporarily deployed to renku-ci-ui-3494 January 23, 2025 13:55 — with GitHub Actions Inactive

andre-code force-pushed the andrea/styles-launch-build branch from 7a096c7 to 630a9de Compare January 23, 2025 14:27

andre-code temporarily deployed to renku-ci-ui-3494 January 23, 2025 14:27 — with GitHub Actions Inactive

andre-code requested a review from leafty January 23, 2025 14:39

leafty reviewed Jan 24, 2025

View reviewed changes

andre-code added 2 commits January 24, 2025 10:46

use isRenkuLegacy function when handle styles

b5cf21a

update e2e tests

8e23d07

andre-code had a problem deploying to renku-ci-ui-3494 January 24, 2025 10:16 — with GitHub Actions Failure

andre-code deployed to renku-ci-ui-3494 January 24, 2025 10:59 — with GitHub Actions View deployment

leafty approved these changes Jan 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: set styles for new routing #3494

feat: set styles for new routing #3494

andre-code commented Jan 20, 2025 •

edited

Loading

RenkuBot commented Jan 20, 2025

leafty left a comment

leafty Jan 22, 2025

andre-code Jan 23, 2025

leafty Jan 22, 2025

leafty Jan 22, 2025

leafty Jan 22, 2025

leafty Jan 22, 2025

andre-code Jan 23, 2025

leafty Jan 22, 2025

leafty Jan 22, 2025

leafty Jan 22, 2025

leafty Jan 22, 2025

andre-code Jan 23, 2025

leafty Jan 22, 2025

andre-code commented Jan 23, 2025

leafty left a comment

leafty Jan 24, 2025

ciyer commented Jan 24, 2025

	<Link to={Url.get(ABSOLUTE_ROUTES.v1.notifications)}>
	<Link to={ABSOLUTE_ROUTES.v1.notifications}>

	<Link className="dropdown-item" to={Url.get(ABSOLUTE_ROUTES.v1.help)}>
	<Link className="dropdown-item" to={ABSOLUTE_ROUTES.v1.help}>

	to={Url.get(ABSOLUTE_ROUTES.v1.root)}
	to={ABSOLUTE_ROUTES.v1.root}

	to={Url.get(ABSOLUTE_ROUTES.v1.search)}
	to={ABSOLUTE_ROUTES.v1.search}

feat: set styles for new routing #3494

Are you sure you want to change the base?

feat: set styles for new routing #3494

Conversation

andre-code commented Jan 20, 2025 • edited Loading

Summary of Changes

Included in the Update:

Pending Fixes to solve in (#3501):

RenkuBot commented Jan 20, 2025

leafty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andre-code commented Jan 23, 2025

leafty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciyer commented Jan 24, 2025

andre-code commented Jan 20, 2025 •

edited

Loading