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.

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

refactor: store accordion preference #815

Merged
merged 30 commits into from
Nov 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
454792d
refactor: set accordions opened by default
patricio0312rev Nov 13, 2024
1bca28f
fix: unit tests
patricio0312rev Nov 13, 2024
c7242a3
refactor: store use accordion preference on local storage
patricio0312rev Nov 13, 2024
5202eb0
refactor: use accordion implementations
patricio0312rev Nov 13, 2024
e420d69
fix: unit tests
patricio0312rev Nov 13, 2024
0788384
fix: unit tests
patricio0312rev Nov 13, 2024
bc982c5
style: resolve style guide violations
patricio0312rev Nov 13, 2024
a950013
fix: unit tests
patricio0312rev Nov 13, 2024
c86f710
Merge branch 'refactor/set-accordions-open-by-default' of https://git…
patricio0312rev Nov 13, 2024
fbd32ff
fix unit tests
patricio0312rev Nov 13, 2024
aab10b2
Merge branch 'refactor/set-accordions-open-by-default' into refactor/…
patricio0312rev Nov 13, 2024
449f343
style: resolve style guide violations
patricio0312rev Nov 13, 2024
87b7851
fix: unit tests
patricio0312rev Nov 13, 2024
1e53ef0
Merge branch 'refactor/set-accordions-open-by-default' into refactor/…
patricio0312rev Nov 13, 2024
7481d0e
Merge branch 'refactor/store-accordion-preference' of https://github.…
patricio0312rev Nov 13, 2024
8230e50
wip
patricio0312rev Nov 13, 2024
ac9be86
Merge branch 'refactor/set-accordions-open-by-default' into refactor/…
patricio0312rev Nov 13, 2024
9196133
fix: wip
patricio0312rev Nov 13, 2024
33d8b7a
wip
patricio0312rev Nov 13, 2024
9f2ed24
fix: unit tests
patricio0312rev Nov 13, 2024
39e8ac0
style: resolve style guide violations
patricio0312rev Nov 13, 2024
5e0d4de
chore: merge with develop
patricio0312rev Nov 14, 2024
0100bf1
Merge branch 'refactor/store-accordion-preference' of https://github.…
patricio0312rev Nov 14, 2024
a481e1b
fix: unit tests
patricio0312rev Nov 14, 2024
ad3a95b
fix: adjust local storage key per profile id
patricio0312rev Nov 14, 2024
d2ec495
refactor: set accordion key per profile
patricio0312rev Nov 14, 2024
7b32045
Merge branch 'develop' into refactor/store-accordion-preference
patricio0312rev Nov 14, 2024
3ab7dd4
chore: eslint
patricio0312rev Nov 14, 2024
5aaa2a3
style: resolve style guide violations
patricio0312rev Nov 14, 2024
8e56c67
Merge branch 'develop' into refactor/store-accordion-preference
shahin-hq Nov 15, 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
4 changes: 2 additions & 2 deletions src/app/components/Accordion/Accordion.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,9 @@ describe("Button", () => {

it("should toggle the accordion on click", async () => {
const Accordion = () => {
useAccordion();
useAccordion("accordion");

const { isExpanded, handleHeaderClick } = useAccordion();
const { isExpanded, handleHeaderClick } = useAccordion("accordion");

return (
<AccordionWrapper>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,57 @@ exports[`Button > should hide the toggle button if no onClick method 1`] = `
</div>
`;

exports[`Button > should not set it expanded if no stored value 1`] = `
<div>
<div
class="css-fvpuky"
>
<div
class="select-none cursor-pointer group md:h-20 py-6 px-8 md:p-4 flex flex-row items-center border-theme-secondary-300 dark:border-theme-secondary-800"
data-testid="AccordionHeader"
>
<div
class="flex flex-grow flex-row items-center"
>
Header
</div>
<div
class="ml-4 flex flex-shrink-0 items-center self-stretch transition-all duration-100 css-o6tyc"
>
<div
class="css-tz6w2"
data-testid="Accordion__toggle"
>
<div
class="transition-transform css-15txs7d"
height="10"
width="10"
>
<svg
id="chevron-down-small"
viewBox="0 0 10 10"
x="0"
xml:space="preserve"
xmlns="http://www.w3.org/2000/svg"
y="0"
>
<path
d="M9 3.1L5.2 6.9c-.1.1-.3.1-.4 0L1 3.1"
fill="none"
stroke="currentColor"
stroke-linecap="round"
stroke-linejoin="round"
stroke-width="2"
/>
</svg>
</div>
</div>
</div>
</div>
</div>
</div>
`;

exports[`Button > should render 1`] = `
<div>
<div
Expand Down
30 changes: 25 additions & 5 deletions src/app/hooks/use-accordion.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,37 @@
import { useCallback, useState } from "react";
import { useCallback, useEffect, useState } from "react";

export const useAccordion = () => {
const [isExpanded, setIsExpanded] = useState(true);
const getStorageKey = (key: string) => `accordion_${key}_isExpanded`;

export const useAccordion = (key: string) => {
const storageKey = getStorageKey(key);
const [isExpanded, setIsExpanded] = useState<boolean>(() => {
const storedValue = localStorage.getItem(storageKey);
return storedValue ? JSON.parse(storedValue) : true;
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can remove the JSON.parse part, because it stores only bool primitives which don't require deserialization, unless your plan was keeping data in a single key.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I am parsing to keep the consistency of the booleans. If I remove this, we could get values like "true", "false" and that could lead to false positives.

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case storing 1 or 0 could resolve the issue 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although it simplifies the issue with "true" and "false", that would reduce readability, and seeing that there are a lot of components and hooks in the repo, I wouldn't take that path 🤔


const handleHeaderClick = useCallback(
(event: React.MouseEvent) => {
event.stopPropagation();
event.preventDefault();

setIsExpanded(!isExpanded);
const newValue = !isExpanded;
setIsExpanded(newValue);
localStorage.setItem(storageKey, JSON.stringify(newValue));
},
[isExpanded],
[isExpanded, storageKey],
);

useEffect(() => {
const handleStorageChange = () => {
const storedValue = localStorage.getItem(storageKey);
if (storedValue) {
setIsExpanded(JSON.parse(storedValue));
}
};

window.addEventListener("storage", handleStorageChange);
return () => window.removeEventListener("storage", handleStorageChange);
}, [storageKey]);

return { handleHeaderClick, isExpanded };
};
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export const ContactListItemMobile: React.VFC<Properties> = ({
[availableNetworks, onSend],
);

const { isExpanded, handleHeaderClick } = useAccordion();
const { isExpanded, handleHeaderClick } = useAccordion(`${profile.id()}_contact_list_mobile_${contact.id()}`);

return (
<AccordionWrapper>
Expand Down
10 changes: 7 additions & 3 deletions src/domains/setting/pages/Servers/Servers.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1000,6 +1000,10 @@ describe("Servers Settings", () => {
},
);

const table = screen.getByTestId(customPeerListTestId);

await userEvent.click(within(table).getAllByTestId(networkAccordionIconTestId)[0]);

await waitFor(() => expect(screen.queryByTestId(peerStatusLoadingTestId)).not.toBeInTheDocument());

await userEvent.click(screen.queryAllByTestId("CustomPeers-network-item--mobile--refresh")[0]);
Expand Down Expand Up @@ -1406,10 +1410,10 @@ describe("Servers Settings", () => {
);

// Is loading initially
expect(screen.getAllByTestId(peerStatusLoadingTestId)).toHaveLength(6);
expect(screen.getAllByTestId(peerStatusLoadingTestId)).toHaveLength(3);

// After ping it should show error
await waitFor(() => expect(screen.getAllByTestId(peerStatusErrorTestId)).toHaveLength(6));
await waitFor(() => expect(screen.getAllByTestId(peerStatusErrorTestId)).toHaveLength(3));

expect(asFragment()).toMatchSnapshot();
});
Expand All @@ -1430,7 +1434,7 @@ describe("Servers Settings", () => {
);

// After ping it should show error
await waitFor(() => expect(screen.getAllByTestId(peerStatusErrorTestId)).toHaveLength(5));
await waitFor(() => expect(screen.getAllByTestId(peerStatusErrorTestId)).toHaveLength(4));

expect(asFragment()).toMatchSnapshot();
});
Expand Down
Loading
Loading