EVG-19921: Allow unhiding patch card #2092

SupaJoon · 2023-10-06T21:12:09Z

EVG-19921

hidden.mov

Evergreen PR

cypress · 2023-10-06T21:37:13Z

Passing run #13734 ↗︎

0	601	7	0	0
⚠️ You've recorded test results over your free plan limit. Upgrade your plan to view test results.

Details:

fix copy
Project: Spruce	Commit: `a87d9a3cdf`
Status: Passed	Duration: 19:05 💡
Started: Nov 8, 2023 5:50 PM	Ended: Nov 8, 2023 6:09 PM

Review all test suite changes for PR #2092 ↗︎

SupaJoon · 2023-10-10T14:08:36Z

I'd like to discuss this feature in the design sync before making more changes.

SupaJoon · 2023-10-11T15:13:33Z

I'll close this PR for now and reopen when it's ready for review

SupaJoon · 2023-10-17T14:45:12Z

This feature is similar to the "Include commit queue"

sophstad · 2023-10-30T19:26:29Z

src/components/PatchesPage/PatchCard.tsx

+  > button {
+    margin-left: ${size.xs};
+  }


Since this is already using flexbox, you can use the gap property instead

Suggested change

> button {

margin-left: ${size.xs};

}

gap: ${size.xs};

sophstad · 2023-10-30T19:29:34Z

src/components/PatchesPage/index.tsx

+const StyledCheckbox = styled(Checkbox)`
+  padding-left: ${size.xs};
+`;


Similar note as above, you can add a gap to CheckboxContainer instead of creating a specialized checkbox.

sophstad · 2023-10-30T19:47:41Z

src/components/PatchesPage/index.tsx

+    e: React.ChangeEvent<HTMLInputElement>
+  ): void => {
+    setIsIncludeHiddenCheckboxChecked(e.target.checked);
+    Cookies.set(INCLUDE_HIDDEN_PATCHES, e.target.checked ? "true" : "false");


When I load this page with this cookie set to true, the checkbox is checked but hidden patches are not visible in the list (i.e. the state is set but the query does not include the hidden parameter).

Tbh I don't know how helpful a cookie is for this setting. Unlike the commit queue, it's probably something that would be looked at in special circumstances, not every time a user visits this page. But if you think it's worth including then the bug should be fixed!

I was able to fix the issue with initial state. I decided to include the cookie to keep consistency with the commit queue checkbox and pagination buttons and also because it's more likely to be helpful than annoying/intrusive

khelif96 · 2023-10-30T13:55:31Z

src/components/PatchesPage/index.tsx

+  ): void => {
+    setIsIncludeHiddenCheckboxChecked(e.target.checked);
+    Cookies.set(INCLUDE_HIDDEN_PATCHES, e.target.checked ? "true" : "false");
+    // eslint-disable-next-line no-unused-expressions


I see this is also disabled above but I'm confused as to why. Can we get rid of it?

It's not necessary and I accidentally copied it in.

khelif96 · 2023-10-30T13:59:32Z

src/components/PatchesPage/index.tsx

@@ -136,12 +160,14 @@ export const usePatchesInputFromSearch = (search: string): PatchesInput => {
    PatchPageQueryParams.Statuses,
    []
  );
+  const [hidden] = useQueryParam(PatchPageQueryParams.Hidden, false);


nit. Can we break this out into a separate file in this folder similar to what we do for other exported hooks.

khelif96 · 2023-10-30T14:01:28Z

src/constants/cookies.ts

Thanks for sorting!

khelif96 · 2023-10-30T14:02:47Z

src/analytics/patches/useProjectPatchesAnalytics.ts

@@ -7,6 +7,7 @@ type Action =
  | { name: "Click Patch Link" }
  | { name: "Click Variant Icon"; variantIconStatus: string }
  | { name: "Filter Commit Queue" }
+  | { name: "Filter Hidden" }


Can we update this analytics event to include what he hidden state is? So that it becomes a little more useful.

khelif96 · 2023-10-30T14:07:33Z

src/components/PatchActionButtons/SetPatchVisibility.tsx

  const { sendEvent } = usePatchAnalytics(patchId);
  const [setPatchVisibility] = useMutation<
    SetPatchVisibilityMutation,
    SetPatchVisibilityMutationVariables
  >(SET_PATCH_VISIBILITY, {
    onCompleted: () => {
-      dispatchToast.success("This patch was successfully hidden");
+      dispatchToast.success("Successfully updated patch visibility.");


I'm actually not a fan of this verbiage, It isn't as clear as the old one. If you update the SET_PATCH_VISIBILITY mutation to also return the hidden state then you should be able to conditionally render a more tailored success message.

khelif96 · 2023-10-31T21:33:47Z

src/components/PatchActionButtons/SetPatchVisibility.tsx

+    onCompleted: (d) => {
+      dispatchToast.success(
+        `This patch was successfully ${
+          d.setPatchVisibility?.[0].hidden ? "unhidden" : "hidden"


nit. Can you destructure this instead of deeply nesting?

setPatchVisibility is used in the outer scope so it can't be destructured in the onCompleted function argument.
The alternative is to do something like this:
const { hidden } = d.setPatchVisibility?.[0] ?? {};
I think this is a lot more busy than what I have originally considering there is an extra assignment and ternary for a variable that's only used once. Instead I can move the copy calculation out of the string literal.

khelif96 · 2023-10-31T21:33:55Z

src/components/PatchesPage/PatchCard.tsx

-  > button {
-    margin-left: ${size.xs};
-  }
+  gap: ${size.xs};


khelif96 · 2023-10-31T21:35:24Z

src/components/PatchesPage/usePatchesQueryParams.ts

+  const [hidden] = useQueryParam(PatchPageQueryParams.Hidden, false);
+  const statuses = rawStatuses.filter((v) => v && v !== ALL_PATCH_STATUS);
+  return {
+    limit: getLimitFromSearch(search),


Since you refactored this hook. Can we just use the useQueryParam hook for everything instead of relying on these older getLimitFromSearch and getPageFromSearch functions. I would like to eventually get rid of them in favor of the hooks.

I think this should be done in a separate ticket because it's not a straightforward refactor and I think the two functions serve their purpose. getLimitFromSearch and getPageFromSearch have reuseable logic that validate and map the query param to a valid value. If these two functions are removed, useQueryParam should accept a validation function that determines whether the default value is returned. Otherwise, it may be worth to create a usePagination hook that includes logic from usePageSizeSelector and calls into useQueryParam since page and limit are reserved for the pagination feature and used in a bunch of places.

khelif96 · 2023-10-31T21:56:17Z

src/components/PatchesPage/PatchCard.tsx

@@ -128,10 +130,12 @@ export const PatchCard: React.FC<Props> = ({
        <TaskBadgeContainer>{badges}</TaskBadgeContainer>
      </Center>
      <Right>
+        {hidden && <Badge data-cy="hidden-badge">Hidden</Badge>}


Was this discussed at the design sync it seems a little out of place? I wonder if we can use something else to represent hidden patches?
Like opacity?

We discussed the Hidden badge at the design sync but not the placement. I prefer the badge instead of opacity because opacity gives the patch card a filter effect and makes it more difficult to read. This is against the users intention to show hidden items.
I aligned Hidden badge next to the menu items because that's where the state is updated.

khelif96 · 2023-11-01T15:57:41Z

src/components/PatchesPage/PatchCard.tsx

nit. Can we move this into the PatchCard folder.

SupaJoon · 2023-11-02T20:17:49Z

evergreen retry

SupaJoon · 2023-11-08T17:09:41Z

evergreen retry

SupaJoon added 3 commits October 6, 2023 16:04

toggle hidden patch view

73a487e

toggle visibility

fadfd27

tests

0f7877a

SupaJoon requested a review from a team October 9, 2023 14:53

SupaJoon closed this Oct 11, 2023

Merge branch 'main' of github.com:evergreen-ci/spruce into EVG-19921

37eadf7

SupaJoon reopened this Oct 16, 2023

SupaJoon added 4 commits October 16, 2023 11:11

onlyHidden -> includeHidden

e3381b1

test cookie

70bcc3c

Merge branch 'main' of github.com:evergreen-ci/spruce into EVG-19921

13da0fb

hidden badge

015bd04

sophstad suggested changes Oct 30, 2023

View reviewed changes

fix initial state

dca96b5

khelif96 reviewed Oct 30, 2023

View reviewed changes

SupaJoon added 6 commits October 30, 2023 16:42

initial state test

afd6cf6

Merge branch 'main' of github.com:evergreen-ci/spruce into EVG-19921

06cbb2a

use gap

b5d67b6

refactor hook

7c59b4f

update analytics

a4ef717

update toast copy

cc9e880

SupaJoon mentioned this pull request Oct 31, 2023

EVG-19921: Set includeHidden to false in Patches query #2123

Closed

SupaJoon requested review from khelif96 and sophstad October 31, 2023 19:41

khelif96 reviewed Nov 1, 2023

View reviewed changes

SupaJoon added 2 commits November 2, 2023 13:42

refactor

edd4699

refactor

773872f

casing

29f3806

SupaJoon requested a review from khelif96 November 2, 2023 18:49

sophstad approved these changes Nov 2, 2023

View reviewed changes

typo

fbf715e

fix copy

a87d9a3

khelif96 removed their request for review November 9, 2023 21:56

SupaJoon merged commit fa2cf39 into evergreen-ci:main Nov 10, 2023
5 checks passed

SupaJoon deleted the EVG-19921 branch November 10, 2023 18:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EVG-19921: Allow unhiding patch card #2092

EVG-19921: Allow unhiding patch card #2092

SupaJoon commented Oct 6, 2023 •

edited

Loading

cypress bot commented Oct 6, 2023 •

edited

Loading

SupaJoon commented Oct 10, 2023

SupaJoon commented Oct 11, 2023

SupaJoon commented Oct 17, 2023

sophstad Oct 30, 2023

sophstad Oct 30, 2023

sophstad Oct 30, 2023 •

edited

Loading

SupaJoon Oct 30, 2023

khelif96 Oct 30, 2023

SupaJoon Oct 31, 2023

khelif96 Oct 30, 2023

khelif96 Oct 30, 2023

khelif96 Oct 30, 2023

khelif96 Oct 30, 2023

khelif96 Oct 31, 2023

SupaJoon Nov 2, 2023

khelif96 Oct 31, 2023

khelif96 Oct 31, 2023

SupaJoon Nov 2, 2023 •

edited

Loading

khelif96 Oct 31, 2023

SupaJoon Nov 2, 2023

khelif96 Nov 1, 2023

SupaJoon commented Nov 2, 2023

SupaJoon commented Nov 8, 2023

EVG-19921: Allow unhiding patch card #2092

EVG-19921: Allow unhiding patch card #2092

Conversation

SupaJoon commented Oct 6, 2023 • edited Loading

Evergreen PR

cypress bot commented Oct 6, 2023 • edited Loading

Passing run #13734 ↗︎

Review all test suite changes for PR #2092 ↗︎

SupaJoon commented Oct 10, 2023

SupaJoon commented Oct 11, 2023

SupaJoon commented Oct 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophstad Oct 30, 2023 • 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

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

SupaJoon Nov 2, 2023 • 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

SupaJoon commented Nov 2, 2023

SupaJoon commented Nov 8, 2023

SupaJoon commented Oct 6, 2023 •

edited

Loading

cypress bot commented Oct 6, 2023 •

edited

Loading

sophstad Oct 30, 2023 •

edited

Loading

SupaJoon Nov 2, 2023 •

edited

Loading