frontend: convert sidebar to functional component #2314

thisconnect · 2023-10-25T14:10:37Z

No description provided.

thisconnect · 2023-10-25T14:29:14Z

frontends/web/src/components/sidebar/sidebar.tsx

    };
-  };
+  }, [sidebarStatus]);


this whole effect could probably become a hook, but keeping it here for now to not have too many changes.

benma · 2023-10-25T14:37:27Z

This is nice but there will be some conflicts with #2283 and #2308.

If you plan more refactorings, let's check first if there will be conflicts in the staging branches to avoid needless rebasing effort.

Now that this is done, of course feel free to merge it and I'll try to resolve the conflicts when pulling master to staging-watchonly.

thisconnect · 2023-10-25T14:40:30Z

There will also be conflicts in staging-ln and staging-bitsurance, both each add another link in the sidebar afaik.

This is nice but there will be some conflicts with #2283 and #2308.

thisconnect · 2023-10-25T14:43:15Z

I think we should refactor and cleanup, before starting new features. Reason that sidebar should be functional is so that we can finally get rid of the share HOC decorators (2 shared stores are left, rates and sidebar state).

EDIT: lets wait for bitsurance and watch-only before we merge this PR. I'll rebase it accordingly once that all is merged.

thisconnect · 2023-12-14T13:27:04Z

rebased

thisconnect · 2023-12-19T10:19:42Z

rebased

Beerosagos · 2023-12-21T09:16:45Z

frontends/web/src/components/sidebar/sidebar.tsx

+      setKeystores(keystores);
+
+    });
+    return subscribeKeystores(setKeystores);


`subscribeKeystore should be called without the return, otherwise the subscription only happens when unmounting the component, right?

no quiet. But I did a short cut here, so it looks a bit confusing. This is the same as:

const unsubscribeFn = subscribeKeystores(keystores => setKeystores(keystores)); return () => { unsibscribeFn(); };

the returned function is only called when the dependency-list (2nd arg of useEffect) is changing or on unmount. In this case (with empty deplist) only on unmount.

oh looking at the code I see your point, sorry. But I tested it and it seemed to me that the sidebar didn't update when connecting/disconneting a new keystore.. I'll check that again.

You were right, it works nicely. Don't know why it seemed different before

If you want I can also rewrite and store the unsubscribe fn in a variable like I did in the code example, for better readability... but I think we do use the shortcut way in a few other places already... let me know.

no no, this is nice!

maybe a comment would be nice for readability, but I think this is fine either way

Beerosagos · 2023-12-21T09:33:08Z

frontends/web/src/components/sidebar/sidebar.tsx

-          </div>
+  const hidden = sidebarStatus === 'forceHidden';
+  const hasOnlyBTCAccounts = accounts.every(({ coinCode }) => isBitcoinOnly(coinCode));
+  const accountsByKeystore = getAccountsByKeystore(accounts);


I guess these don't need a `useEffect' hook as the whole component is re-rendered when the account list changes, right?

Yes these lines are currently executed on every render, like before this commit (those lines were just in render function).

But there is some potential for optimization here, as hasOnlyBTCAccounts and accountsByKeystore only depend on accounts. So we probably should move this so that accounts.every( and getAccountsByKeystore(accounts) only is executed when accounts change. This way it would not do the operations on every render.

Beerosagos · 2023-12-21T09:44:12Z

frontends/web/src/components/sidebar/sidebar.tsx

+              title={t('accountSummary.title')}
+              onClick={handleSidebarItemClick}>
+              <div className={style.single}>
+                <img draggable={false} src={info} alt={t('sidebar.addAccount')} />


alt text looks wrong

thanks for catching this, I accidentally re-added during rebase. I'll remove the alt attribute and fixup the last commit

Beerosagos · 2023-12-21T09:54:39Z

frontends/web/src/components/sidebar/sidebar.tsx

-                <img draggable={false} src={settings} alt={t('sidebar.settings')} />
+              to="/buy/info">
+              <div className={style.single}>
+                <img draggable={false} src={coins} alt={t('sidebar.exchanges')} />


sidebar.exchanges doesn't seem to exist in the locales file. This was already like this on master, though.

Beerosagos

tACK! Very nice

Note the Sidebar component is still using share HOC decorator, but this can now be easier replaced with react context. Also remove 2 unused translation strings on image alt attributes that had no corresponding translations.

thisconnect force-pushed the frontend-sidebar-functional branch from 17c7cf6 to e9dd306 Compare October 25, 2023 14:23

thisconnect changed the title ~~Frontend sidebar functional~~ frontend: convert sidebar to functional component Oct 25, 2023

thisconnect force-pushed the frontend-sidebar-functional branch from e9dd306 to 276abad Compare October 25, 2023 14:24

thisconnect commented Oct 25, 2023

View reviewed changes

thisconnect force-pushed the frontend-sidebar-functional branch from 276abad to 16e0a10 Compare December 14, 2023 13:26

thisconnect force-pushed the frontend-sidebar-functional branch 2 times, most recently from f550c2c to 855186d Compare December 19, 2023 10:19

thisconnect requested review from shonsirsha and removed request for shonsirsha December 19, 2023 10:19

thisconnect force-pushed the frontend-sidebar-functional branch from eb78710 to 855186d Compare December 20, 2023 16:24

thisconnect requested a review from Beerosagos December 20, 2023 16:29

Beerosagos requested changes Dec 21, 2023

View reviewed changes

thisconnect force-pushed the frontend-sidebar-functional branch from 855186d to c165f87 Compare December 21, 2023 10:34

Beerosagos approved these changes Dec 21, 2023

View reviewed changes

frontend: convert sidebar to functional component

a8be91a

Note the Sidebar component is still using share HOC decorator, but this can now be easier replaced with react context. Also remove 2 unused translation strings on image alt attributes that had no corresponding translations.

thisconnect force-pushed the frontend-sidebar-functional branch from c165f87 to a8be91a Compare December 21, 2023 13:35

thisconnect merged commit 6a07734 into BitBoxSwiss:master Dec 21, 2023

thisconnect deleted the frontend-sidebar-functional branch April 18, 2025 10:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

frontend: convert sidebar to functional component #2314

frontend: convert sidebar to functional component #2314

thisconnect commented Oct 25, 2023 •

edited

Loading

thisconnect Oct 25, 2023

benma commented Oct 25, 2023

thisconnect commented Oct 25, 2023

thisconnect commented Oct 25, 2023 •

edited

Loading

thisconnect commented Dec 14, 2023

thisconnect commented Dec 19, 2023

Beerosagos Dec 21, 2023

thisconnect Dec 21, 2023

Beerosagos Dec 21, 2023

Beerosagos Dec 21, 2023

thisconnect Dec 21, 2023

Beerosagos Dec 21, 2023

Beerosagos Dec 21, 2023

Beerosagos Dec 21, 2023

thisconnect Dec 21, 2023

Beerosagos Dec 21, 2023

thisconnect Dec 21, 2023

thisconnect Dec 21, 2023

Beerosagos Dec 21, 2023

thisconnect Dec 21, 2023

Beerosagos left a comment

frontend: convert sidebar to functional component #2314

frontend: convert sidebar to functional component #2314

Conversation

thisconnect commented Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

benma commented Oct 25, 2023

thisconnect commented Oct 25, 2023

thisconnect commented Oct 25, 2023 • edited Loading

thisconnect commented Dec 14, 2023

thisconnect commented Dec 19, 2023

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

Beerosagos left a comment

Choose a reason for hiding this comment

thisconnect commented Oct 25, 2023 •

edited

Loading

thisconnect commented Oct 25, 2023 •

edited

Loading