[Refactor] Sidebar cleanup (and a bit more that's related) #3544

arielj · 2024-02-10T17:21:38Z

This PR is a refactor to cleanup the code related to the Sidebar component.

I does a few things:

Extracted a new SidebarItem component, so we don't need to use <button> and NavLink directly in SidebarLinks (this helps remove a lot of duplication around creating those items)
Replaced the .scss files with .css, we are not really using any sass feature
Refactored the css files to use native css nesting and moved the styles to the corresponding components' css (we had a lot of css in the main sidebar.css file that is easier to find in each component's css, we also had css for Sidebar__item in multiple css files)
I added new CSS properties specific to the sidebar that can help when theming, they fallback to what we already had so nothing changes, but new themes can use better variables that don't cause unexpected side effect
Because of this cleanup, I had to remove the state: { fromGameCard: ...} that was not really used anymore since the settings for a game were moved to the SettingsModal component. This led into some changes needed also in the useSettingsContext context and the Settings screen to remove the code that was using that

How to test/qa this

The sidebar should behave the same way as before (I didn't really change any CSS, I just moved things to different places and added nesting, and the new custom properties have fallbacks to the previous values):

Resizing
Hiding content during resize
sizes, colors, etc depending on the theme
etc

Use the following Checklist if you have changed something on the Backend or Frontend:

Tested the feature and it's working on a current and clean install.
Tested the main App features and they are still working on a current and clean install. (Login, Install, Play, Uninstall, Move games, etc.)
Created / Updated Tests (If necessary)
Created / Updated documentation (If necessary)

arielj · 2024-02-10T17:22:59Z

src/frontend/components/UI/Sidebar/components/CurrentDownload/index.css

Most of this file is the previous index.scss file renamed and adding the .Sidebar { nesting, and some new code extracted from the Sidebar/index.css file that fits better here

arielj · 2024-02-10T17:23:37Z

src/frontend/components/UI/Sidebar/components/CurrentDownload/index.tsx

+          </Box>
+        </Box>
+      </div>
+    </Link>


This is just me removing the extra <> fragment that was not needed, the diff is just the indentation

arielj · 2024-02-10T17:24:29Z

src/frontend/components/UI/Sidebar/components/HeroicVersion/index.css

The code here is just CSS extracted from Sidebar/index.css, and refactored to use nesting

arielj · 2024-02-10T17:25:27Z

src/frontend/components/UI/Sidebar/components/SidebarItem/index.css

+      --navbar-active,
+      var(--accent-overlay, var(--accent))
+    );
+    --sidebar-active-item-background: var(--navbar-active-background);


this file is mostly extraction of Sidebar__item styles that were in other places, but I also added these new custom properties to help themes

arielj · 2024-02-10T17:27:39Z

src/frontend/components/UI/Sidebar/index.css

+    margin-block: 0 var(--sidebar-vertical-padding);
+    margin-inline: var(--sidebar-horizontal-padding) var(--space-3xs);
+    -webkit-app-region: no-drag;
+  }


I'm still keeping these types of properties here in the parent and not in the component's css because these styles should be controlled by the parent since they are styles "outside" the component

arielj · 2024-02-10T17:29:09Z

src/frontend/components/UI/Winetricks/index.tsx

@@ -21,7 +21,7 @@ export default function Winetricks({ onClose }: Props) {
    setLoading(true)
    try {
      const components = await window.api.winetricksListInstalled(
-        runner,
+        runner!,


This is not needed because runner can be undefined in the Settings Context, but cannot be undefined in this case (we would never show the winetricks option anywhere without a game, so runner would be defined

It'd be more robust if you'd pass the runner via this component's props (and then only take a Runner, not a Runner | undefined). You'd then not need these assertions

Assertions like this are dangerous, since if we ever change the conditions for showing this component (to no longer look at the context's runner), this will silently break

I'll updated the if (....) that we have above in this component to not render anything if we don't have a runner (we already render <></> when isDefault which implies runner is undefined, but making that explicit will get rid of these !)

arielj · 2024-02-10T17:30:39Z

src/frontend/screens/Settings/index.tsx

-              className="backButton"
-              state={{ gameInfo: gameInfo }}
-            >
+            <NavLink to="/" role="link" className="backButton">


Since we moved the settings for specific games into a modal, this was always / anyway, we don't need all the code to detect to go back to a game page because that doesn't happen anymore

This should go to /library for now (until #3540 is merged). Navigating to / just navigates to /library with an extra re-render

CommandMC · 2024-02-12T14:16:34Z

src/frontend/screens/Settings/index.tsx

-              className="backButton"
-              state={{ gameInfo: gameInfo }}
-            >
+            <NavLink to="/" role="link" className="backButton">


This should go to /library for now (until #3540 is merged). Navigating to / just navigates to /library with an extra re-render

CommandMC · 2024-02-12T14:19:13Z

src/frontend/components/UI/Sidebar/components/SidebarLinks/index.tsx

-                </span>
-              </NavLink>
+              <SidebarItem
+                url="/settings/app/default/general"


This url seems incorrect; shouldn't it be /settings/app/default/games_settings? Right now, both the "General" and "Game Defaults" buttons link to the same place

yep, wrong link, I'll actually do a bit more refactoring, we don't really need the app/default part of these urls anymore

arielj · 2024-02-12T15:01:52Z

src/backend/main.ts

@@ -1676,7 +1676,7 @@ ipcMain.on('processShortcut', async (e, combination: string) => {
      break
    // hotkey to open the settings on frontend
    case 'ctrl+k':
-      sendFrontendMessage('openScreen', '/settings/app/default/general')
+      sendFrontendMessage('openScreen', '/settings/general')


we don't need these app/default routes anymore, the Settings component always renders Heroic's settings, game's settings are not handled by this url

arielj · 2024-02-12T15:06:17Z

src/frontend/screens/Settings/components/Tools/index.tsx

    return <></>
  }

  type Tool = 'winecfg' | string
-  async function callTools(tool: Tool, exe?: string) {
+  const callTools = async (tool: Tool, exe?: string) => {


I had to change these to arrow functions because the typescript complains about runner being possible undefined inside function() (even though this happens after the if)

looks like, because this is different in the 2 cases, runner can be undefined for named functions but it's correct with arrow functions

arielj · 2024-02-12T15:07:19Z

src/frontend/screens/Settings/index.tsx

  })

  // render `loading` while we fetch the settings
-  if (!title || !contextValues) {
+  if (!currentConfig || !contextValues) {


we were checking title here, but title doesn't really change, it's better to check for currentConfig since it's what the comment is saying originally and that's the real intention

flavioislima

LGTM

…to a modal

…sed for heroic's settings now

arielj added the pr:ready-for-review Feature-complete, ready for the grind! :P label Feb 10, 2024

arielj requested review from a team, flavioislima, CommandMC, Etaash-mathamsetty, Nocccer and imLinguin and removed request for a team February 10, 2024 17:21

arielj commented Feb 10, 2024

View reviewed changes

CommandMC reviewed Feb 12, 2024

View reviewed changes

arielj commented Feb 12, 2024

View reviewed changes

arielj requested a review from CommandMC February 16, 2024 18:49

arielj mentioned this pull request Apr 2, 2024

[UI/UX] Improve sidebar #3671

Draft

4 tasks

flavioislima approved these changes Nov 18, 2024

View reviewed changes

arielj added 8 commits December 13, 2024 22:37

Delete settings scren code unused since the game settings were moved …

2ce0e45

…to a modal

These changes are needed for the previous commit too

15293ca

Create a SidebarItem to cleanup the SidebarLinks component

1850db1

Update/cleanup CSS for all sidebar components

c521950

Tell typescript these places do have a valid runner

9c28fbc

Fix linter offense

818f20f

Add translation back

733cc2f

Remove deadcode

4ad21a6

arielj added 4 commits December 13, 2024 22:41

Don't do runner!. Fix game_defaults url. Use /library.

1ae89d7

remove app/default routes for settings, since that screen is only u…

dbabe6e

…sed for heroic's settings now

More cleanup of unused code

41b5007

Fix settings url after rebase

4786c30

arielj force-pushed the refactor-sidebar-cleanup branch from be86363 to 4786c30 Compare December 14, 2024 01:52

Fix test clicking Settings in sidebar

79e4c48

arielj merged commit a1a6f4a into main Dec 14, 2024
9 checks passed

arielj deleted the refactor-sidebar-cleanup branch December 14, 2024 02:22

Heroic-Games-Launcher locked and limited conversation to collaborators Dec 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Refactor] Sidebar cleanup (and a bit more that's related) #3544

[Refactor] Sidebar cleanup (and a bit more that's related) #3544

arielj commented Feb 10, 2024 •

edited

Loading

arielj Feb 10, 2024

arielj Feb 10, 2024

arielj Feb 10, 2024

arielj Feb 10, 2024 •

edited

Loading

arielj Feb 10, 2024

arielj Feb 10, 2024

CommandMC Feb 12, 2024

arielj Feb 12, 2024

arielj Feb 10, 2024

CommandMC Feb 12, 2024

CommandMC Feb 12, 2024

CommandMC Feb 12, 2024

arielj Feb 12, 2024

arielj Feb 12, 2024

arielj Feb 12, 2024

arielj Feb 12, 2024

flavioislima left a comment

[Refactor] Sidebar cleanup (and a bit more that's related) #3544

[Refactor] Sidebar cleanup (and a bit more that's related) #3544

Conversation

arielj commented Feb 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielj Feb 10, 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

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

flavioislima left a comment

Choose a reason for hiding this comment

arielj commented Feb 10, 2024 •

edited

Loading

arielj Feb 10, 2024 •

edited

Loading