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
Show file tree
Hide file tree
Changes from all 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>;
}
Expand Down Expand Up @@ -86,7 +74,7 @@ export class App<
}
>
<Provider store={store}>
<AppRouter />
<RouterProvider router={router} />
</Provider>
</ThemeProvider>
</PrefContext.Provider>
Expand Down
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";
Expand All @@ -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
Expand Down Expand Up @@ -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));
gabalafou marked this conversation as resolved.
Show resolved Hide resolved
environmentNotification({
data: {
show: true,
description: createLabel(name, "create")
}
});
dispatch(showNotification(createLabel(name, "create")));
} catch (e) {
setError({
message: e?.data?.message ?? createLabel(undefined, "error"),
Expand Down
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";
Expand All @@ -8,7 +10,9 @@
import { useGetBuildQuery } from "../environmentDetailsApiSlice";
import {
updateEnvironmentBuildId,
environmentClosed
environmentClosed,
environmentOpened,
toggleNewEnvironmentView
} from "../../../features/tabs";
import {
useGetBuildPackagesQuery,
Expand All @@ -34,30 +38,81 @@
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 = change of

Check failure on line 93 in src/features/environmentDetails/components/EnvironmentDetails.tsx

View workflow job for this annotation

GitHub Actions / Build Package

Delete `·`
// (namespaceName, environmentName) in the URL
// 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();
peytondmurray marked this conversation as resolved.
Show resolved Hide resolved

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

useGetEnviromentBuildsQuery(selectedEnvironment, {
useGetEnviromentBuildsQuery(selectedEnvironment ?? skipToken, {
peytondmurray marked this conversation as resolved.
Show resolved Hide resolved
pollingInterval: INTERVAL_REFRESHING
});

Expand Down Expand Up @@ -112,7 +167,7 @@
};

const loadArtifacts = async () => {
if (artifactType.includes("DOCKER_MANIFEST")) {
if (!currentBuildId || artifactType.includes("DOCKER_MANIFEST")) {
peytondmurray marked this conversation as resolved.
Show resolved Hide resolved
return;
}

Expand All @@ -122,7 +177,7 @@
};

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

Expand Down Expand Up @@ -171,12 +226,7 @@
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:
Expand All @@ -187,7 +237,7 @@
visible: true
});
}
scrollRef.current.scrollTo(0, 0);
scrollRef.current?.scrollTo(0, 0);
};

const updateBuild = async (buildId: number) => {
Expand All @@ -201,12 +251,9 @@
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"),
Expand All @@ -232,19 +279,17 @@
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);
};

Expand All @@ -255,10 +300,15 @@
})();
}, INTERVAL_REFRESHING);

if (!selectedEnvironment) {
return null;
}

return (
<Box sx={{ padding: "15px 12px" }}>
<EnvironmentDetailsHeader
envName={name}
namespace={namespace?.name}
onUpdateName={setName}
showEditButton={selectedEnvironment?.canUpdate}
/>
Expand Down
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";
Expand All @@ -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;
Expand All @@ -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
Expand Down
Loading
Loading