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.

Sign up for GitHub

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

Jump to bottom

Bye tabs, hi URLs #389

Merged
merged 9 commits into from
May 1, 2024
Merged

Bye tabs, hi URLs #389

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
db3d2c4
Rename AppExample.tsx to main.tsx and use createBrowserRouter
gabalafou Mar 31, 2024
11eb62d
app URL routing
gabalafou Apr 1, 2024
7a736af
todo comments
gabalafou Apr 9, 2024
d47a430
Apply suggestions from code review
gabalafou Apr 10, 2024
e46eccc
Apply suggestions from code review
gabalafou Apr 10, 2024
f5a9fd9
fix test
gabalafou Apr 10, 2024
0c3e0c8
simplify namespace + environment hook
gabalafou Apr 30, 2024
8116d1d
fix playwright test
gabalafou Apr 30, 2024
2fda414
Update src/features/environmentDetails/components/EnvironmentDetails.tsx
gabalafou May 1, 2024
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
17 changes: 3 additions & 14 deletions src/App.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,32 +1,21 @@
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>
);
};

Check failure on line 18 in src/App.tsx

View workflow job for this annotation

GitHub Actions / Build Package 

Delete `⏎`
gabalafou marked this conversation as resolved.
Show resolved Hide resolved
export interface IAppProps {
pref?: Partial<IPreferences>;
}
Expand Down Expand Up @@ -86,7 +75,7 @@
}
>
<Provider store={store}>
<AppRouter />
<RouterProvider router={router} />
</Provider>
</ThemeProvider>
</PrefContext.Provider>
Expand Down
39 changes: 25 additions & 14 deletions src/features/environmentCreate/components/EnvironmentCreate.tsx
View file
Open in desktop
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,12 @@ export const EnvironmentCreate = ({ environmentNotification }: IEnvCreate) => {
dispatch(
environmentOpened({
environment,
selectedEnvironmentId: newEnvId,
canUpdate: true
})
);
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
102 changes: 69 additions & 33 deletions src/features/environmentDetails/components/EnvironmentDetails.tsx
View file
Open in desktop
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 { SpecificationEdit, SpecificationReadOnly } from "./Specification";
import { useGetBuildQuery } from "../environmentDetailsApiSlice";
import {
updateEnvironmentBuildId,
environmentClosed
environmentClosed,
environmentOpened,
toggleNewEnvironmentView
} from "../../../features/tabs";
import {
useGetBuildPackagesQuery,
Expand All @@ -34,30 +38,67 @@ 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 foundNamespace = Boolean(namespace);
const environments: Environment[] = useAppSelector(
state => state.environments.data
);
const environment = environments.find(
environment =>
environment.namespace.name === namespaceName &&
environment.name === environmentName
);
const foundEnvironment = Boolean(environment);
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.

}
}, [namespaceName, environmentName, foundNamespace, foundEnvironment]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's probably not clear what I'm doing here. I'll need to add a comment or something.

I want this React hook to run whenever the namespace and environment name changes, but I also need it to re-run when an environment or namespace have been found.

Previously this component (EnvironmentDetails) was only rendered once the environment details had been fetched from the server (see the old version of PageLayout in the files changed).

Now the component renders before the data has been fetched from the server. So this hook will run once but nothing will happen because namespace and environment are undefined. Then once those are fetched, the hook will run again.

Most of the changes to this component (including passing the Redux toolkit skip token, checking for current build id, and the check for selected environment) have to do with it now being rendered before all the data has arrived.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, you're right that a comment here would help clear this up. I guess this means the UI will now have an "intermediate" state where this hook has run once but not twice, right?

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 think I overcomplicated this. I'm going to push a commit that should simplify this.


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 +122,7 @@ export const EnvironmentDetails = ({
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 +153,7 @@ export const EnvironmentDetails = ({
};

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 +163,7 @@ export const EnvironmentDetails = ({
};

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

Expand Down Expand Up @@ -171,12 +212,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:
Expand All @@ -187,7 +223,7 @@ export const EnvironmentDetails = ({
visible: true
});
}
scrollRef.current.scrollTo(0, 0);
scrollRef.current?.scrollTo(0, 0);
};

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

Expand All @@ -255,10 +286,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}
/>
Expand Down
6 changes: 3 additions & 3 deletions src/features/environments/components/Environment.tsx
View file
Open in desktop
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