-
Notifications
You must be signed in to change notification settings - Fork 21
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
Bye tabs, hi URLs #389
Changes from 4 commits
db3d2c4
11eb62d
7a736af
d47a430
e46eccc
f5a9fd9
0c3e0c8
8116d1d
2fda414
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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,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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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( | ||
|
@@ -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 | ||
}); | ||
|
||
|
@@ -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; | ||
} | ||
|
||
|
@@ -122,7 +163,7 @@ export const EnvironmentDetails = ({ | |
}; | ||
|
||
const loadDependencies = async () => { | ||
if (dependencies.length) { | ||
if (!currentBuildId || dependencies.length) { | ||
return; | ||
} | ||
|
||
|
@@ -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: | ||
|
@@ -187,7 +223,7 @@ export const EnvironmentDetails = ({ | |
visible: true | ||
}); | ||
} | ||
scrollRef.current.scrollTo(0, 0); | ||
scrollRef.current?.scrollTo(0, 0); | ||
}; | ||
|
||
const updateBuild = async (buildId: number) => { | ||
|
@@ -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"), | ||
|
@@ -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); | ||
}; | ||
|
||
|
@@ -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} | ||
/> | ||
|
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} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
to={`/${environment.namespace.name}/${environment.name}`} | ||
sx={{ | ||
color: isSelected | ||
? theme.palette.primary.main | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🤷
There was a problem hiding this comment.
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.)