Bye tabs, hi URLs #389

gabalafou · 2024-04-09T15:51:54Z

Closes #386

Testing

The automatic tests should give some confidence that this PR does not horribly break anything. That said, a manual run-through should be done before approving and merging.

Types of website actions affected by this PR that should be tested manually in various combinations:

Open an environment
Create a new environment
Go back
Go forward
Click edit
Click save
Click cancel
Click delete

gabalafou

done self-reviewing

gabalafou · 2024-04-09T15:53:38Z

webpack.config.js

  watch: false,
  ...getContext(__dirname),

  output: {
    path: path.resolve(__dirname, "dist"),
-    filename: "[name].js"
+    filename: "[name].js",
+    publicPath: "/"


Had to add this to get the dev server to work. Will need to check that this works for production server too. Full disclosure: webpack is black magic to me.

😅 Really not sure why either. I guess this is telling webpack to insert / at the beginning of every path?

I guess we'll just need to keep an eye on this when we release and deploy

test/playwright/test_ux.py

test/environments/Environment.test.tsx

gabalafou · 2024-04-09T15:57:58Z

test/environmentDetails/EnvironmentDetailsHeader.test.tsx

+    // const editButton = component.getByText("Edit");
+    // fireEvent.click(editButton);
+    // expect(store.getState().environmentDetails.mode).toEqual("edit");


TODO remove or fix

src/utils/helpers/namespaces.ts

gabalafou · 2024-04-10T20:37:52Z

src/layouts/PageLayout.tsx


-  const onUpdateOrCreateEnv = ({ data }: IUpdateEnvironment) => {


I decided to get rid of this callback function because:

Turning the child components into routed components means that the parent no longer imports the child components and so cannot pass properties directly to them (see line 47 on the right for how a route parent has to pass properties to route children)

It seemed that having a centralized place in the app state for notifications is a good idea. PageLayout probably shouldn't own these functions.

Keeping the setRefreshEnvironment behavior would be easy with the useEffect hook. However this will need to be changed in the future (I will add a TODO comment)

src/routes.tsx

src/store/rootReducer.ts

src/App.tsx

src/features/environments/reducer.ts

gabalafou · 2024-04-10T20:57:28Z

src/features/environments/components/Environment.tsx

@@ -44,7 +43,8 @@ export const Environment = ({
        />
      </ListItemIcon>
      <Button
-        onClick={onClick}
+        component={Link}


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.

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

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.)

src/features/environments/components/EnvironmentDropdown.tsx

src/features/environmentCreate/components/EnvironmentCreate.tsx

peytondmurray

Tested locally, seems like it's working. I'm a big fan of putting state into the URL, so this seems like a really nice change. Thank you! 🎉

peytondmurray · 2024-04-25T17:39:20Z

src/features/environmentCreate/components/EnvironmentCreate.tsx

+      dispatch(modeChanged(EnvironmentDetailsModes.CREATE));
+      dispatch(openCreateNewEnvironmentTab(namespaceName));


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 🤷

peytondmurray · 2024-04-25T18:16:23Z

src/features/environmentDetails/components/EnvironmentDetails.tsx

+      dispatch(modeChanged(EnvironmentDetailsModes.READ));
+      dispatch(toggleNewEnvironmentView(false));
+    }
+  }, [namespaceName, environmentName, foundNamespace, foundEnvironment]);


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?

peytondmurray · 2024-04-25T18:28:54Z

src/features/environments/components/Environment.tsx

@@ -44,7 +43,8 @@ export const Environment = ({
        />
      </ListItemIcon>
      <Button
-        onClick={onClick}
+        component={Link}


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

peytondmurray · 2024-04-25T18:30:42Z

src/features/environments/components/EnvironmentDropdown.tsx

  const { palette } = useTheme();

-  const onCreateNewEnvironmentTab = (


peytondmurray · 2024-04-25T18:44:11Z

src/layouts/PageLayout.tsx

+  // This use of state to refetch the environment is an anti-pattern. We should
+  // leverage RTK's cache invalidation features instead.
+  useEffect(() => {


Instead of this comment, can you create an issue so we can track this work later on? That way we won't lose track of it after we merge this PR.

Done:

Anti-pattern: setState() to trigger environment refetch #397

src/routes.tsx

test/environments/Environment.test.tsx

test/playwright/test_ux.py

peytondmurray · 2024-04-25T19:13:39Z

webpack.config.js

  watch: false,
  ...getContext(__dirname),

  output: {
    path: path.resolve(__dirname, "dist"),
-    filename: "[name].js"
+    filename: "[name].js",
+    publicPath: "/"


😅 Really not sure why either. I guess this is telling webpack to insert / at the beginning of every path?

gabalafou

Thanks @peytondmurray for your review!

gabalafou · 2024-04-30T10:30:54Z

src/features/environmentCreate/components/EnvironmentCreate.tsx

+      dispatch(modeChanged(EnvironmentDetailsModes.CREATE));
+      dispatch(openCreateNewEnvironmentTab(namespaceName));


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.)

gabalafou · 2024-04-30T10:46:06Z

src/features/environments/components/Environment.tsx

@@ -44,7 +43,8 @@ export const Environment = ({
        />
      </ListItemIcon>
      <Button
-        onClick={onClick}
+        component={Link}


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.)

src/routes.tsx

gabalafou · 2024-04-30T15:21:55Z

src/features/environmentDetails/components/EnvironmentDetails.tsx

+      dispatch(modeChanged(EnvironmentDetailsModes.READ));
+      dispatch(toggleNewEnvironmentView(false));
+    }
+  }, [namespaceName, environmentName, foundNamespace, foundEnvironment]);


I think I overcomplicated this. I'm going to push a commit that should simplify this.

gabalafou · 2024-04-30T15:26:24Z

src/layouts/PageLayout.tsx

+  // This use of state to refetch the environment is an anti-pattern. We should
+  // leverage RTK's cache invalidation features instead.
+  useEffect(() => {


Done:

Anti-pattern: setState() to trigger environment refetch #397

src/features/environmentDetails/components/EnvironmentDetails.tsx

gabalafou added 2 commits April 2, 2024 19:09

Rename AppExample.tsx to main.tsx and use createBrowserRouter

db3d2c4

app URL routing

11eb62d

gabalafou changed the title ~~React router~~ Bye tabs, hi URLs Apr 9, 2024

todo comments

7a736af

trallard added the type: enhancement 💅🏼 New feature or request label Apr 9, 2024

trallard requested a review from peytondmurray April 9, 2024 16:33

gabalafou commented Apr 10, 2024

View reviewed changes

Apply suggestions from code review

d47a430

gabalafou commented Apr 10, 2024

View reviewed changes

src/features/environmentCreate/components/EnvironmentCreate.tsx Outdated Show resolved Hide resolved

gabalafou added 2 commits April 10, 2024 16:22

Apply suggestions from code review

e46eccc

fix test

f5a9fd9

trallard added this to the challenges: Round 2 🚀 milestone Apr 16, 2024

peytondmurray approved these changes Apr 25, 2024

View reviewed changes

simplify namespace + environment hook

0c3e0c8

gabalafou commented Apr 30, 2024

View reviewed changes

fix playwright test

8116d1d

gabalafou commented May 1, 2024

View reviewed changes

src/features/environmentDetails/components/EnvironmentDetails.tsx Outdated Show resolved Hide resolved

Update src/features/environmentDetails/components/EnvironmentDetails.tsx

2fda414

gabalafou merged commit 0526a09 into conda-incubator:main May 1, 2024
3 of 4 checks passed

gabalafou deleted the react-router branch May 1, 2024 08:26

gabalafou added a commit to gabalafou/conda-store-ui that referenced this pull request May 7, 2024

Bye tabs, hi URLs (conda-incubator#389)

0d550fd

This was referenced May 8, 2024

Store environment form values in hash object #400

Closed

Create environment details are reset when the page loses scope. #241

Open

Give each environment its own edit form state in the UI app global state (Redux) #394

Open

kcpevey mentioned this pull request Jun 19, 2024

[ENH] - Upgrade to conda-store 2024.10.1 nebari-dev/nebari#2523

Open

gabalafou added a commit to gabalafou/conda-store-ui that referenced this pull request Jul 9, 2024

Bye tabs, hi URLs (conda-incubator#389)

19cc689

This was referenced Aug 6, 2024

[BUG] - Clicking on different environment tabs changes the visible build #333

Open

[BUG] - Active build isn't updated in the UI until environment tab is closed #334

Open

[BUG] - Unable to access tab close button with many tabs open #382

Closed

trallard mentioned this pull request Sep 6, 2024

DEV - Improve bundling and release #418

Merged

3 tasks

gabalafou mentioned this pull request Oct 3, 2024

Memory routing to fix JupyterLab extension #429

Merged

2 tasks

This was referenced Oct 18, 2024

Add config option REACT_APP_URL_BASENAME #431

Merged

Valid sub-routes for the UI app result in 404 conda-incubator/conda-store#899

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bye tabs, hi URLs #389

Bye tabs, hi URLs #389

gabalafou commented Apr 9, 2024 •

edited

Loading

gabalafou left a comment

gabalafou Apr 9, 2024

peytondmurray Apr 25, 2024 •

edited

Loading

gabalafou Apr 30, 2024

gabalafou Apr 9, 2024

gabalafou Apr 10, 2024

gabalafou Apr 10, 2024

peytondmurray Apr 25, 2024

gabalafou Apr 30, 2024

peytondmurray left a comment

peytondmurray Apr 25, 2024

peytondmurray Apr 25, 2024

peytondmurray Apr 25, 2024

peytondmurray Apr 25, 2024

peytondmurray Apr 25, 2024

gabalafou Apr 30, 2024 •

edited

Loading

peytondmurray Apr 25, 2024 •

edited

Loading

gabalafou left a comment

gabalafou Apr 30, 2024

gabalafou Apr 30, 2024

gabalafou Apr 30, 2024

gabalafou Apr 30, 2024 •

edited

Loading


		const onUpdateOrCreateEnv = ({ data }: IUpdateEnvironment) => {

		dispatch(modeChanged(EnvironmentDetailsModes.CREATE));
		dispatch(openCreateNewEnvironmentTab(namespaceName));

		const { palette } = useTheme();

		const onCreateNewEnvironmentTab = (

Bye tabs, hi URLs #389

Bye tabs, hi URLs #389

Conversation

gabalafou commented Apr 9, 2024 • edited Loading

Testing

gabalafou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peytondmurray Apr 25, 2024 • edited Loading

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

peytondmurray 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

gabalafou Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

peytondmurray Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

gabalafou 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

gabalafou Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

gabalafou commented Apr 9, 2024 •

edited

Loading

peytondmurray Apr 25, 2024 •

edited

Loading

gabalafou Apr 30, 2024 •

edited

Loading

peytondmurray Apr 25, 2024 •

edited

Loading

gabalafou Apr 30, 2024 •

edited

Loading