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

Bye tabs, hi URLs #389

Merged
merged 9 commits into from
May 1, 2024
Merged
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 3 additions & 15 deletions src/App.tsx
Original file line number Diff line number Diff line change
@@ -1,32 +1,20 @@
import { ThemeProvider } from "@mui/material";
import React from "react";
import { Provider } from "react-redux";
import { BrowserRouter as Router } from "react-router-dom";
import { Route, Routes } from "react-router";
import { RouterProvider } from "react-router-dom";

import { PageLayout } from "./layouts";
import {
IPreferences,
PrefContext,
prefDefault,
prefGlobal
} from "./preferences";
import { router } from "./routes";
import { store } from "./store";
import { condaStoreTheme, grayscaleTheme } from "./theme";

import "../style/index.css";

const AppRouter = () => {
// for now, trivial routing is sufficient
return (
<Router>
<Routes>
<Route path="*" element={<PageLayout />} />
</Routes>
</Router>
);
};

export interface IAppProps {
pref?: Partial<IPreferences>;
}
@@ -86,7 +74,7 @@ export class App<
}
>
<Provider store={store}>
<AppRouter />
<RouterProvider router={router} />
</Provider>
</ThemeProvider>
</PrefContext.Provider>
40 changes: 26 additions & 14 deletions src/features/environmentCreate/components/EnvironmentCreate.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import React, { useState } from "react";
import React, { useEffect, useState } from "react";
import { useNavigate, useParams } from "react-router-dom";
import Box from "@mui/material/Box";
import Alert from "@mui/material/Alert";
import { stringify } from "yaml";
@@ -16,23 +17,38 @@ import {
} from "../../../features/metadata";
import {
environmentOpened,
closeCreateNewEnvironmentTab
closeCreateNewEnvironmentTab,
openCreateNewEnvironmentTab
} from "../../../features/tabs";
import { useAppDispatch, useAppSelector } from "../../../hooks";
import { SpecificationCreate, SpecificationReadOnly } from "./Specification";
import { descriptionChanged, nameChanged } from "../environmentCreateSlice";
import createLabel from "../../../common/config/labels";

export interface IEnvCreate {
environmentNotification: (notification: any) => void;
}
import { showNotification } from "../../notification/notificationSlice";

interface ICreateEnvironmentArgs {
code: { channels: string[]; dependencies: string[] };
}

export const EnvironmentCreate = ({ environmentNotification }: IEnvCreate) => {
export const EnvironmentCreate = () => {
const dispatch = useAppDispatch();

// Url routing params
// If user loads the app at /<namespace_name>/new-environment
// This will put the app in the correct state
const { namespaceName } = useParams<{
namespaceName: string;
}>();

useEffect(() => {
if (namespaceName) {
dispatch(modeChanged(EnvironmentDetailsModes.CREATE));
dispatch(openCreateNewEnvironmentTab(namespaceName));
}
Comment on lines +45 to +46
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These dispatch calls used to happen when the user clicked the button. Now we need to dispatch these actions when the user navigates to the URL. I put these dispatch calls in a useEffect hook so that they don't cause infinite re-renders (by caching on the value of namespaceName on line 48 below).

I don't really love using useEffect this way but I couldn't think of a better cost-benefit way to do it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, I'm not even sure how else you'd do it other than a useEffect hook unless you mean something really hacky with URLSearchParams or something. So this seems good 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean, I could put some bit of code that runs as soon as the the Redux-related functions are available, checks the URL, and dispatches these actions. Or I could probably figure out some way to leverage the React Router's loader function.

But so long as this seems good to you, I'll keep it for now. (Again, it's a question of effort versus reward, and I'm not sure more effort is worth the reward, not yet.)

}, [namespaceName]);

const navigate = useNavigate();

const { mode } = useAppSelector(state => state.environmentDetails);
const { name, description } = useAppSelector(
state => state.environmentCreate
@@ -84,17 +100,13 @@ export const EnvironmentCreate = ({ environmentNotification }: IEnvCreate) => {
dispatch(
environmentOpened({
environment,
selectedEnvironmentId: newEnvId,
canUpdate: true
})
);
// After new environment has been created, navigate to the new environment's web page
navigate(`/${namespace}/${name}`);
dispatch(currentBuildIdChanged(data.build_id));
environmentNotification({
data: {
show: true,
description: createLabel(name, "create")
}
});
dispatch(showNotification(createLabel(name, "create")));
} catch (e) {
setError({
message: e?.data?.message ?? createLabel(undefined, "error"),
116 changes: 83 additions & 33 deletions src/features/environmentDetails/components/EnvironmentDetails.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import React, { useEffect, useState } from "react";
import { skipToken } from "@reduxjs/toolkit/query/react";
import { useNavigate, useParams } from "react-router-dom";
import Box from "@mui/material/Box";
import Alert from "@mui/material/Alert";
import { stringify } from "yaml";
@@ -8,7 +10,9 @@ import { SpecificationEdit, SpecificationReadOnly } from "./Specification";
import { useGetBuildQuery } from "../environmentDetailsApiSlice";
import {
updateEnvironmentBuildId,
environmentClosed
environmentClosed,
environmentOpened,
toggleNewEnvironmentView
} from "../../../features/tabs";
import {
useGetBuildPackagesQuery,
@@ -34,30 +38,81 @@ import artifactList from "../../../utils/helpers/artifact";
import createLabel from "../../../common/config/labels";
import { AlertDialog } from "../../../components/Dialog";
import { useAppDispatch, useAppSelector } from "../../../hooks";
import { CondaSpecificationPip } from "../../../common/models";
import {
CondaSpecificationPip,
Environment,
Namespace
} from "../../../common/models";
import { useInterval } from "../../../utils/helpers";
import { showNotification } from "../../notification/notificationSlice";
import { useScrollRef } from "../../../layouts/PageLayout";

interface IEnvDetails {
scrollRef: any;
environmentNotification: (notification: any) => void;
}
interface IUpdateEnvironmentArgs {
dependencies: (string | CondaSpecificationPip)[];
channels: string[];
}

const INTERVAL_REFRESHING = 5000;

export const EnvironmentDetails = ({
scrollRef,
environmentNotification
}: IEnvDetails) => {
export const EnvironmentDetails = () => {
const dispatch = useAppDispatch();

// Url routing params
// If user loads the app at /<namespace_name>/<environment_name>
// This will put the app in the correct state
const { namespaceName, environmentName } = useParams<{
namespaceName: string;
environmentName: string;
}>();
const namespaces: Namespace[] = useAppSelector(
state => state.namespaces.data
);
const namespace = namespaces.find(({ name }) => name === namespaceName);
const environments: Environment[] = useAppSelector(
state => state.environments.data
);
const environment = environments.find(
environment =>
environment.namespace.name === namespaceName &&
environment.name === environmentName
);
useEffect(() => {
if (namespace && environment) {
dispatch(
environmentOpened({
environment,
canUpdate: namespace.canUpdate
})
);
dispatch(modeChanged(EnvironmentDetailsModes.READ));
dispatch(toggleNewEnvironmentView(false));
Comment on lines +81 to +88
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These actions used to be dispatched on click when the user clicked on an environment, but now they need to happen when the user navigates to an environment's web page.

}
}, [
// We only want to run this effect when:
//
// 1. User navigates to different environment - i.e., (namespaceName,
// environmentName) in the URL change
// 2. The corresponding (namespace, environment) data have been fetched
//
// We cannot pass [namespace, environment] as the dependencies to
// useEffect() because whenever an environment is created or updated, a
// refetch of the data is triggered, which creates new data objects for
// [namespace, environment] in the Redux store, which would cause this
// effect to rerun, but, again, we only want to run this effect when the
// user navigates to a new (namespaceName, environmentName), hence the `&&
// namespace.name` and `&& environment.name`.
namespace && namespace.name,
environment && environment.name
]);

const navigate = useNavigate();

const { mode } = useAppSelector(state => state.environmentDetails);
const { page, dependencies } = useAppSelector(state => state.dependencies);
const { selectedEnvironment } = useAppSelector(state => state.tabs);
const { currentBuild } = useAppSelector(state => state.enviroments);
const [name, setName] = useState(selectedEnvironment?.name || "");
const scrollRef = useScrollRef();

const [descriptionIsUpdated, setDescriptionIsUpdated] = useState(false);
const [description, setDescription] = useState(
@@ -81,7 +136,7 @@ export const EnvironmentDetails = ({
const [updateBuildId] = useUpdateBuildIdMutation();
const [deleteEnvironment] = useDeleteEnvironmentMutation();

useGetEnviromentBuildsQuery(selectedEnvironment, {
useGetEnviromentBuildsQuery(selectedEnvironment ?? skipToken, {
pollingInterval: INTERVAL_REFRESHING
});

@@ -112,7 +167,7 @@ export const EnvironmentDetails = ({
};

const loadArtifacts = async () => {
if (artifactType.includes("DOCKER_MANIFEST")) {
if (!currentBuildId || artifactType.includes("DOCKER_MANIFEST")) {
return;
}

@@ -122,7 +177,7 @@ export const EnvironmentDetails = ({
};

const loadDependencies = async () => {
if (dependencies.length) {
if (!currentBuildId || dependencies.length) {
return;
}

@@ -171,12 +226,7 @@ export const EnvironmentDetails = ({
dispatch(modeChanged(EnvironmentDetailsModes.READ));
setCurrentBuildId(data.build_id);
dispatch(currentBuildIdChanged(data.build_id));
environmentNotification({
data: {
show: true,
description: createLabel(environment, "update")
}
});
dispatch(showNotification(createLabel(environment, "update")));
} catch (e) {
setError({
message:
@@ -187,7 +237,7 @@ export const EnvironmentDetails = ({
visible: true
});
}
scrollRef.current.scrollTo(0, 0);
scrollRef.current?.scrollTo(0, 0);
};

const updateBuild = async (buildId: number) => {
@@ -201,12 +251,9 @@ export const EnvironmentDetails = ({
buildId
}).unwrap();
dispatch(updateEnvironmentBuildId(buildId));
environmentNotification({
data: {
show: true,
description: createLabel(selectedEnvironment.name, "updateBuild")
}
});
dispatch(
showNotification(createLabel(selectedEnvironment.name, "updateBuild"))
);
} catch (e) {
setError({
message: createLabel(undefined, "error"),
@@ -232,19 +279,17 @@ export const EnvironmentDetails = ({
selectedEnvironmentId: selectedEnvironment.id
})
);
environmentNotification({
data: {
show: true,
description: createLabel(selectedEnvironment.name, "delete")
}
});
dispatch(
showNotification(createLabel(selectedEnvironment.name, "delete"))
);
navigate("/");
} catch (e) {
setError({
message: createLabel(undefined, "error"),
visible: true
});
}
scrollRef.current.scrollTo(0, 0);
scrollRef.current?.scrollTo(0, 0);
setShowDialog(false);
};

@@ -255,10 +300,15 @@ export const EnvironmentDetails = ({
})();
}, INTERVAL_REFRESHING);

if (!selectedEnvironment) {
return null;
}

return (
<Box sx={{ padding: "15px 12px" }}>
<EnvironmentDetailsHeader
envName={name}
namespace={namespace?.name}
onUpdateName={setName}
showEditButton={selectedEnvironment?.canUpdate}
/>
6 changes: 3 additions & 3 deletions src/features/environments/components/Environment.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import React from "react";
import { Link } from "react-router-dom";
import CircleIcon from "@mui/icons-material/Circle";
import ListItemIcon from "@mui/material/ListItemIcon";
import Button from "@mui/material/Button";
@@ -12,13 +13,11 @@ interface IEnvironmentProps {
* @param selectedEnvironmentId id of the currently selected environment
*/
environment: EnvironmentModel;
onClick: () => void;
selectedEnvironmentId: number | undefined;
}

export const Environment = ({
environment,
onClick,
selectedEnvironmentId
}: IEnvironmentProps) => {
const isSelected = selectedEnvironmentId === environment.id;
@@ -44,7 +43,8 @@ export const Environment = ({
/>
</ListItemIcon>
<Button
onClick={onClick}
component={Link}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to change the Button component directly to a Link, but it was causing styling issues. The reason why is that the MUI button component renders with several classes attached to it and those same classes get applied whether the HTML element used is button or (now) a. This should probably be cleaned up at some point but it's low priority in my mind.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yah, this doesn't seem like a big deal unless I'm missing something accessibility related or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's just not very hygienic. Because you end up with an <a> tag that has button classes applied to it. (But yes, not a big deal.)

to={`/${environment.namespace.name}/${environment.name}`}
sx={{
color: isSelected
? theme.palette.primary.main
Loading
Loading