Add remove app button #989

joshri · 2021-10-29T01:26:50Z

Closes: #904

The biggest changes here are in the ApplicationDetail page, where a remove app button has been added along with error handling for a failed GitHub authentication. You can see the functionality below!

Screen.Recording.2021-10-28.at.8.26.14.PM.mov

NOTE: I already fixed the wonky spacing of the Error Alert since the creation of the video -__-

Some additional styling changes to get things looking right: addition of topRight prop to the breadcrumbs section to add remove button, size adjustments in Layout to prevent content from overflowing horizontally, and customPadding and customMargin options added to Spacer.

The other big change is in the backend to include the namespace as part of the DELETE request.

One big thing I want to change: I'd like to tie the github authentication success to clearing the authentication error on the first modal. My solution for now is to close the first modal on clicking the authenticate with github button, but that requires the user to make an extra click after returning from github - Jordan I think you managed to do this on the Add Application button - do you think it's possible here as well? I also need to change the ugliness of the loading circle within the button.

jpellizzari · 2021-10-29T16:29:06Z

@joshri Can you edit the title of this PR to read like a commit message/release note? Here are some examples: https://github.com/weaveworks/weave-gitops/releases/tag/v0.3.3

jpellizzari · 2021-10-29T16:32:51Z

@joshri What about if we created a RemoveApp page to handle all the authentication state? Maybe we are forcing too much on the ApplicationDetail page.

So clicking the Remove Application button would navigate to the remove app page, where we can do all the GH auth stuff without losing the context.

WDYT (what do you think)?

jpellizzari

Looks like some other branch or commit made it into this one. LMK if you need help getting your git state corrected.

jpellizzari · 2021-10-29T16:29:51Z

.github/workflows/test.yml

@@ -134,7 +134,7 @@ jobs:

  acceptance-tests-0:
    runs-on: ubuntu-latest
-    needs: [lint, build, test]
+    needs: [lint, build]


? Whats up with these changes?

Don't know what I'm doing 😦 not sure how I caused that!

jpellizzari · 2021-10-29T16:33:22Z

Makefile

@@ -169,10 +169,10 @@ coverage/merged.lcov: coverage/lcov.info coverage/golang.info
 ##@ Utilities

 .PHONY: help
-# Thanks to https://www.thapaliya.com/en/writings/well-documented-makefiles/
+# Thanks to https://www.thapaliya.com/en/writings/well-documented-s/


The node_modules thing I'm almost positive we did on purpose - the parts where makefile is deleted are certainly........suspicious

README.md

jpellizzari · 2021-10-29T16:34:10Z

cmd/gitops/add/app/cmd.go

@@ -1,4 +1,4 @@
-package add
+package app


? It looks like some other changes leaked into your branch

jpellizzari · 2021-10-29T16:35:13Z

ui/components/Layout.tsx

  height: 100%;
  margin: 0 auto;
  padding: 0;
  background-color: ${negativeSpaceColor};
 `;

 const NavContainer = styled.div`
-  width: 240px;
+  min-width: 200px;


I wouldn't have expected layout params to change. This side-bar being variable width will cause some issues.

NavContainer was a set width, but ContentContainer was not - which caused weird interactions with elements inside ContentContainer being able to overflow ContentContainer itself. I found that setting the x-overflow on AppContainer kept ContentContainer and NavContainer in check, and got the percentages working as I expected them too - this might be possible without making changes in Layout but I thought that we wouldn't want x-overflow in almost all situations anyway. Now the min-width...I can't exactly remember WHY I did it that way but I'm sure I had a brilliant and well-thought-out reason - will revert it and see what happens

Please take another look at this. I would really like this to be a fixed width, media queries notwithstanding

jpellizzari · 2021-10-29T16:39:18Z

ui/pages/ApplicationDetail.tsx

+  React.useEffect(() => {
+    if (!removeRes) return;
+    //if app is succesfully removed, redirect to applications page
+    history.push(defaultLinkResolver(PageRoute.Applications));


We need to pull the linkResolver from AppContext. That way, if another context provider gives a different linkResolver, the navigation will work.

jpellizzari · 2021-10-29T16:41:44Z

ui/pages/ApplicationDetail.tsx

+  const RemoveAppAuthError = (
+    <div>
+      {removeError?.code === 16 ? (
+        !authSuccess && (


This is confusing. It would be better to build this into the above conditional.

Suggested change

!authSuccess && (

removeError?.code === 16 && !authSuccess && ()

Yeah...this was my solution to not being able to reset the error - I have to still display an error that's not authentication related, but if authentication has actually happened but the error still exists, display nothing. That probably sounds like nonsense.

ui/pages/ApplicationDetail.tsx

jpellizzari · 2021-10-29T16:44:28Z

ui/pages/Applications.tsx

-      <Flex align wide between>
-        <TitleBar>
+      <Flex align between>
+        <TitleBar style={{ width: "25%" }}>


Re inline styles:
https://c.tenor.com/PLpe-LYpJUcAAAAC/shame-gameofthrones.gif

Not sure if this is left over, but it probably won't work for us. Why wouldn't this be full width?

Having it full width ends up wrapping the button text. I kind of wish everything had a width prop or something like that where I don't have to make a whole class or component to change one style (usually width or height) you know?

Something else is going on if the text inside the button is wrapping because of another component's width.

I kind of wish everything had a width prop

That would be inline styles...

Wow did I just invent inline styles? Great minds. What's the best way for me to add one line of style to something? The TitleBar width is initially 100% and the Button isn't set so it takes up the minimum amount of space.

Inline style would be fine here, but I disagree with the 25% value. I would have to play around and figure out the right values. Maybe <Flex align end /> instead of <Flex align between /> would give you what you want?

I think it's the Link component that is limiting the width of the button - made a class for it.

… Makefile

joshri · 2021-10-29T21:13:43Z

@jpellizzari Here's an updated vid from the latest commit featuring an upgraded UX!

Screen.Recording.2021-10-29.at.5.09.53.PM.mov

jpellizzari · 2021-11-01T14:28:51Z

@joshri How does it know you are not authenticated BEFORE you click remove?

joshri · 2021-11-02T14:05:56Z

It knows because of the 'authSuccess' state variable we're using to check the auth for commit history. I'm just reusing that variable!

jpellizzari · 2021-11-02T14:38:55Z

It knows because of the 'authSuccess' state variable we're using to check the auth for commit history. I'm just reusing that variable!

Oh very slick...

jpellizzari

LGTM, no major issues. Nice work! This was a hard one.

jpellizzari · 2021-11-02T17:24:28Z

ui/components/Layout.tsx

  height: 100%;
  margin: 0 auto;
  padding: 0;
  background-color: ${negativeSpaceColor};
 `;

 const NavContainer = styled.div`
-  width: 240px;
+  min-width: 200px;


Please take another look at this. I would really like this to be a fixed width, media queries notwithstanding

ui/components/Page.tsx

ui/pages/ApplicationDetail.tsx

jpellizzari · 2021-11-02T17:29:16Z

ui/pages/ApplicationDetail.tsx

+        open={removeAppModalOpen}
+        onClose={() => setRemoveAppModalOpen(false)}
+        title="Are You Sure?"
+        description={`You are about to remove ${application.name} from Weave GitOps!`}


Suggested change

description={`You are about to remove ${application.name} from Weave GitOps!`}

description={`You are about to remove ${application.name} from Weave GitOps`}

…on variable

joshri · 2021-11-02T20:04:28Z

@jpellizzari Got all those fixes in the latest commit! Thanks!

bia

LGTM @joshri 🚀
Thank you for the videos, they are very helpful.

Just 2 comments :

Is the pink colour of the remove app button used anywhere else in weave-gitops so far? I know that it is a material design recommendation, but I would prefer Weave Cloud's orange800
The second dialog ('Authenticate with GitHub') shifts down in position while it is loading, maybe it could just have the same y position as the first one ('Are You Sure?')

joshri · 2021-11-03T17:54:56Z

orange800 is looking a little dark -

Here's orange600 -

Here's orange500 -

@bia what do you think?

bia · 2021-11-03T18:16:53Z

Good catch! I like orange600

…ded modal and circular progress height to avoid jittering when removing app

jpellizzari · 2021-11-03T22:32:14Z

ui/components/GithubDeviceAuthModal.tsx

@@ -117,7 +117,7 @@ function GithubDeviceAuthModal({

      <Flex wide center>
        {codeLoading || !codeRes ? (
-          <CircularProgress />
+          <CircularProgress size={143} />


Sort of a random value here. Can we pick some sizes that will keep things consistent? Small, Medium, Large?

It's the value only because that's the exact size of the text that replaces it - I could wrap both in a div with a set size which would definitely be better

ui/components/Layout.tsx

jpellizzari · 2021-11-03T22:33:57Z

ui/components/Modal.tsx

  background-color: ${(props) => props.theme.colors.white};
  margin: 0 auto;
+  height: 400px;


Do we want ALL Modals to have this height?

Yeah...the github auth and remove app modals need to be the same height. I could make a class for the two of them which again, is probably better

jpellizzari · 2021-11-03T22:37:03Z

ui/pages/ApplicationDetail.tsx

+                      applicationsClient.RemoveApplication({
+                        name: application.name,
+                        namespace: application.namespace,
+                        //CAN'T FIND AUTOMERGE IN APPLICATION OBJECT


Why is this comment yelling at me? 😉

Can you phrase this as a note for the next person to come across this code?

I wrote that so I would remember and then I forgot about it which is an absolute classic. Will do.

ui/pages/ApplicationDetail.tsx

…d added option to position alerts icenter rather than flex-start

* Allow for body to be passed in HTTP delete method for RemoveApplication * Add Remove App button * Update README with command npm run test -- -u to update css snapshots * reverted Spacer and Makefile, and changed Remove App confirm text * Reorganize remove app modal display to improve ux and actually revert Makefile * removed inline style on Application TitleBar * update snapshots * small cleanup - text changes, nav bar width, and rremoving extra button variable * added orange600 as the value for secondary in the mui theme, chard coded modal and circular progress height to avoid jittering when removing app * update snapshots * adjusted modal size to account for error alert in GithubAuthModal, and added option to position alerts icenter rather than flex-start Co-authored-by: Jordan Pellizzari <jordan@weave.works> Co-authored-by: Joshua Israel <joshua.israel@weave.works>

Allow for body to be passed in HTTP delete method for RemoveApplication

e040295

joshri requested a review from jpellizzari October 29, 2021 01:26

joshri added the area/ui Issues that require front-end work label Oct 29, 2021

joshri changed the title ~~904 remove app button~~ Add remove app button Oct 29, 2021

jpellizzari reviewed Oct 29, 2021

View reviewed changes

joshri added 2 commits October 29, 2021 10:09

Add Remove App button

37514fc

Update README with command npm run test -- -u to update css snapshots

13c4c61

jpellizzari force-pushed the 904-remove-app-button branch from 11ee60c to 13c4c61 Compare October 29, 2021 17:19

joshri added 2 commits October 29, 2021 13:58

reverted Spacer and Makefile, and changed Remove App confirm text

bdafb55

Reorganize remove app modal display to improve ux and actually revert…

497656c

… Makefile

removed inline style on Application TitleBar

f1d4173

joshri marked this pull request as ready for review November 2, 2021 17:05

joshri requested a review from bia November 2, 2021 17:05

joshri and others added 2 commits November 2, 2021 13:08

Merge branch 'main' into 904-remove-app-button

5b58164

update snapshots

7594f8a

jpellizzari approved these changes Nov 2, 2021

View reviewed changes

small cleanup - text changes, nav bar width, and rremoving extra butt…

19188f3

…on variable

bia suggested changes Nov 3, 2021

View reviewed changes

joshri added 2 commits November 3, 2021 14:52

added orange600 as the value for secondary in the mui theme, chard co…

0977298

…ded modal and circular progress height to avoid jittering when removing app

update snapshots

653d679

jpellizzari approved these changes Nov 3, 2021

View reviewed changes

adjusted modal size to account for error alert in GithubAuthModal, an…

4dce807

…d added option to position alerts icenter rather than flex-start

joshri merged commit a7a1fa4 into main Nov 4, 2021

joshri deleted the 904-remove-app-button branch November 4, 2021 15:42

	!authSuccess && (
	removeError?.code === 16 && !authSuccess && ()

	description={`You are about to remove ${application.name} from Weave GitOps!`}
	description={`You are about to remove ${application.name} from Weave GitOps`}

Add remove app button #989

Add remove app button #989

Conversation

joshri commented Oct 29, 2021 • edited

jpellizzari commented Oct 29, 2021

jpellizzari commented Oct 29, 2021

jpellizzari 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

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

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

joshri commented Oct 29, 2021

jpellizzari commented Nov 1, 2021

joshri commented Nov 2, 2021 • edited

jpellizzari commented Nov 2, 2021

jpellizzari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshri commented Nov 2, 2021

bia left a comment

Choose a reason for hiding this comment

joshri commented Nov 3, 2021 • edited

bia commented Nov 3, 2021

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

joshri commented Oct 29, 2021 •

edited

joshri commented Nov 2, 2021 •

edited

joshri commented Nov 3, 2021 •

edited