refactor: settings dialog #4265

Schwehn42 · 2024-06-26T08:38:14Z

Description

Refactors the settings dialog. The reason for this is the fact the settings will be required at other locations (like templates/sessions, #4254), and while the current state was great, it had some issues:

hardcoded menu items
state that wouldn't be available on other pages (like board state)
hardcoded navigation

The new solution provides solutions for all of these:

dynamically render menu items
1. all sections are kept in a configurable file
2. each section can be enabled/disabled using props
remove or outsource unnessecary state
dynamic navigation
1. returning to previous route instead of navigatin to hardcoded routes
2. when navigating to /settings, automatically redirect to the first allowed option

Changelog

add file constants/settings.ts incuding nessecary types and list of all menu items
SettingsDialog
- add render function
- adjust logic
add tests

Checklist

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
The application was tested in the most commonly used browsers (e.g. Chrome, Firefox, Safari)

for participants, appearance, and profile

# Conflicts: # src/components/SettingsDialog/ShareSession/ShareSession.tsx

octomind-dev · 2024-07-24T11:47:40Z

🐙 Octomind

Test Report: 14/14 successful.

description	status	details
About Section Visibility Test	Passed ✅	click
change avatar	Passed ✅	click
check Privacy Policy	Passed ✅	click
check terms & conditions	Passed ✅	click
close cookie banner - front page	Passed ✅	click
close cookie banner - sign-in	Passed ✅	click
create and delete board columns	Passed ✅	click
create_and_delete_notes_and_actions_v2	Passed ✅	click
create lean coffee board	Passed ✅	click
edit_notes_and_actions_v5	Passed ✅	click
get started	Passed ✅	click
share_session	Passed ✅	click
sign-in	Passed ✅	click
test all ways to open the setup flow	Passed ✅	click

commit sha: 0e85826

brandstetterm · 2024-08-28T11:20:30Z

.eslintrc.json

@@ -57,7 +57,8 @@
    "jsx-a11y/no-noninteractive-element-interactions": "off",
    "no-plusplus": "off",
    "no-param-reassign": "off",
-    "jsx-a11y/anchor-is-valid": "off"
+    "jsx-a11y/anchor-is-valid": "off",
+    "@typescript-eslint/no-unused-vars": ["error", { "argsIgnorePattern": "^_" }]


you're right, this was required for something but not anymore.

brandstetterm · 2024-08-28T11:37:28Z

src/components/Avatar/Avatar.tsx

I'm having an issue wrapping my head around that component right now, but not necessarily because of your changes.

You've made the seed property optional. When the component gets rendered with an undefined seed, you return null. That signals to me that the seed property is necessary in any case to render the component. But there's one case where the seed isn't necessary, isn't it? If the avatar property is defined, we don't need to have the seed to render an avatar. So as I understand it, null should only be returned if the seed and the avatar are undefined.

The change stems from the fact that the seed which is used is the user's id, which can be undefined.
So you could either do a non-null assertion or allow undefined values. I know it's not optimal, maybe it would be a good change overall to change ParticipantsState.me to always be defined?

brandstetterm · 2024-08-28T11:48:02Z

src/components/BoardHeader/HeaderMenu/BoardOptions/BoardOptionButton.tsx

-export const BoardOptionButton: FC<PropsWithChildren<BoardOptionButtonProps>> = ({label, icon, onClick, isExpandable = false, className, children, ...other}) => {
+export const BoardOptionButton: FC<PropsWithChildren<BoardOptionButtonProps>> = ({label, icon, onClick, isExpandable = false, children, ...other}) => {


I guess you've removed the className here because it wasn't used anyways. But having className within the {...other} props make it unusable. If in the future, someone would add a classname to the <BoardOptionButton /> component, it wouldn't be applied.

yeah true, I've readded the prop and also apply it to the div now, which wasn't the case before.

brandstetterm · 2024-08-28T11:50:06Z

src/components/SettingsDialog/Appearance/Appearance.tsx

-    <div className={classNames("settings-dialog__container", "accent-color__lean-lilac")}>
+    <div className={classNames("settings-dialog__container", getColorClassName(activeMenuItem?.color))}>


Isn't the accent color of the appearance tab the same, no matter if it's used within a board or for the template page?

Same question for all of the other tabs

well, since the color of the menu items is determined by the menu item config, it makes sense for the actual settings dialogs to use the same color instead of using hard-coded values.

brandstetterm · 2024-08-28T11:57:51Z

src/routes/Router.tsx

-          <Route path="feedback" element={<Feedback />} />
-          <Route path="profile" element={<ProfileSettings />} />
+const Router = () => {
+  const feedbackEnabled = useAppSelector((state) => state.view.feedbackEnabled);


Doesn't seem like something that belongs in the state (since it won't change), but rather as an imported const with the value of an environment variable.

the value comes from the backend, that's why it's in the store.

brandstetterm · 2024-08-28T11:58:52Z

src/constants/settings.ts

+import {GeneralSettings, Participants, SettingsAppearance, SettingsFeedback, Share, FileDefault} from "components/Icon";
+import {Color} from "./colors";
+
+export const MOBILE_BREAKPOINT = 920;


Where is the magic value coming from?

that magic value has been in the code since the very first version of the setting component added in 2022 (1f88c5e), I just decided to outsource it and clarify its use in the code.

brandstetterm · 2024-08-28T12:13:38Z

src/constants/settings.ts

+export type MenuKey = "board" | "participants" | "appearance" | "share" | "export" | "feedback" | "profile";
+
+type LocalizationKey = "BoardSettings" | "Participants" | "Appearance" | "ShareSession" | "ExportBoard" | "Feedback" | "Profile";
+
+export type MenuItem = {
+  localizationKey: LocalizationKey;
+  location: string;
+  isModeratorOnly: boolean;
+  color: Color;
+  icon: ComponentType<SVGProps<SVGSVGElement>> | "profile"; // profile: special case
+};


I think I'd prefer the following naming: MenuItemKey, MenuItemConfig

renamed 👍

brandstetterm · 2024-08-28T12:14:42Z

src/constants/settings.ts

+export const ENABLE_ALL: Record<MenuKey, boolean> = {
+  board: true,
+  participants: true,
+  appearance: true,
+  share: true,
+  export: true,
+  feedback: true,
+  profile: true,
+};


Suggestion: Add a feedbackEnabled const in this file, get the value from an environment variable and use the value of it inside the ENABLE_ALL record.

Also, maybe a different name like enabledFeedbackItems would fit better

The feedbackEnabled comes from the store which in turn retrieves it from the ServerInfo.

The idea of the const ENABLE_ALL was to be able to use it as a shorthand to avoid having to enable all menu items manually, while still retaining the option to overwrite it.

<SettingsDialog enabledMenuItems={{...ENABLE_ALL, feedback: feedbackEnabled}} />

Remember, the reason this refactor was required in the first place is to be able to dynamically change which menu items are available, and which aren't.

also actually apply it

`MenuKey` -> `MenuItemKey` `MenuItem` -> `MenuItemConfig` `MENU_ENTRY_CONFIG` -> `MENU_ITEM_CONFIG`

github-actions · 2024-09-11T11:17:31Z

The deployment to the dev cluster was successful. You can find the deployment here: https://4265.development.scrumlr.fra.ics.inovex.io
This deployment is only for testing purposes and will be deleted after 1 week.
To redeploy rerun the workflow.
DO NOT STORE IMPORTANT DATA ON THIS DEPLOYMENT

Deployed Images

ghcr.io/inovex/scrumlr.io/scrumlr-frontend:sha-7771ce4
ghcr.io/inovex/scrumlr.io/scrumlr-server:sha-7771ce4

Schwehn42 added 27 commits June 23, 2024 15:15

add settings constants

76088cc

add render function

b4cd75e

render menu items (except profile)

1348e3e

forgot to add export board item

fce7530

make seed prop optional and add safety guard

2d23263

add profile to menu items

57ad812

render profile using new function

059f038

useLocation to highlight active menu item

6935c06

add comments

daca4bc

move functions around

33800f7

use keys

52d950e

render only if enabled

6cd99eb

add shorthand to enable all menu items

60caa91

add prop to router with feedback state

9a3bbeb

readd and fix logic

bbcd7ab

allow blank args

65cbeda

fix key name

f2b64db

simplify

8be188b

move logic

dadd12d

fix mobile breakpoint edge case

dd3a9b1

comment

bbc6502

function to determine first valid menu item

1b32adc

move inside useEffect

c04a117

remove default value to allow reload

ad71b9e

fix test

dceea23

add tests

00cf840

fix import path

f8b8114

Schwehn42 added the Review Needed This pull request is ready for review label Jun 26, 2024

fix unrelated linting warnings

48edcbe

This comment has been minimized.

Sign in to view

Schwehn42 added 11 commits July 17, 2024 15:56

simplify and comment

69d5711

rename functions

42df10b

inline function

15b8201

clarify comment

2afa89a

state setter function typo

fd1cb92

use outlet context for accent color

85936c2

make context value optional

f37d5b0

update snapshots

48051c6

use correct colors

f21f66c

for participants, appearance, and profile

update snapshot again

93a98d3

Merge branch 'main' into js/refactor-settings-dialog

a16da02

This comment has been minimized.

Sign in to view

Schwehn42 added Review Needed This pull request is ready for review and removed Review Needed This pull request is ready for review labels Jul 19, 2024

Schwehn42 added 5 commits July 22, 2024 14:31

Merge branch 'refs/heads/main' into js/refactor-settings-dialog

6c75c54

fix feedback color

4a45b59

Merge branch 'refs/heads/main' into js/refactor-settings-dialog

02aa94e

# Conflicts: # src/components/SettingsDialog/ShareSession/ShareSession.tsx

Merge branch 'refs/heads/main' into js/refactor-settings-dialog

f08c197

snapshot

47e8012

brandstetterm reviewed Aug 28, 2024

View reviewed changes

brandstetterm removed the Review Needed This pull request is ready for review label Aug 28, 2024

Schwehn42 added the Changes Requested Changes requested by the reviewer label Aug 29, 2024

Schwehn42 added 4 commits September 11, 2024 11:19

Merge branch 'main' into js/refactor-settings-dialog

db1e11c

revert eslint rule

024a149

readd className to BoardOptionButton

41f7e17

also actually apply it

rename types/constants

31fda31

`MenuKey` -> `MenuItemKey` `MenuItem` -> `MenuItemConfig` `MENU_ENTRY_CONFIG` -> `MENU_ITEM_CONFIG`

Schwehn42 added Review Needed This pull request is ready for review and removed Changes Requested Changes requested by the reviewer labels Sep 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: settings dialog #4265

refactor: settings dialog #4265

Schwehn42 commented Jun 26, 2024

This comment has been minimized.

This comment has been minimized.

octomind-dev bot commented Jul 24, 2024 •

edited

Loading

brandstetterm Aug 28, 2024

Schwehn42 Sep 11, 2024

brandstetterm Aug 28, 2024

Schwehn42 Sep 11, 2024

brandstetterm Aug 28, 2024

Schwehn42 Sep 11, 2024

brandstetterm Aug 28, 2024

brandstetterm Aug 28, 2024

Schwehn42 Sep 11, 2024

brandstetterm Aug 28, 2024 •

edited

Loading

Schwehn42 Sep 11, 2024

brandstetterm Aug 28, 2024

Schwehn42 Sep 11, 2024

brandstetterm Aug 28, 2024

Schwehn42 Sep 11, 2024

brandstetterm Aug 28, 2024

brandstetterm Aug 28, 2024

Schwehn42 Sep 11, 2024

github-actions bot commented Sep 11, 2024

		export const BoardOptionButton: FC<PropsWithChildren<BoardOptionButtonProps>> = ({label, icon, onClick, isExpandable = false, className, children, ...other}) => {
		export const BoardOptionButton: FC<PropsWithChildren<BoardOptionButtonProps>> = ({label, icon, onClick, isExpandable = false, children, ...other}) => {

		<div className={classNames("settings-dialog__container", "accent-color__lean-lilac")}>
		<div className={classNames("settings-dialog__container", getColorClassName(activeMenuItem?.color))}>

refactor: settings dialog #4265

Are you sure you want to change the base?

refactor: settings dialog #4265

Conversation

Schwehn42 commented Jun 26, 2024

Description

Changelog

Checklist

This comment has been minimized.

This comment has been minimized.

octomind-dev bot commented Jul 24, 2024 • edited Loading

🐙 Octomind

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

brandstetterm Aug 28, 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

github-actions bot commented Sep 11, 2024

octomind-dev bot commented Jul 24, 2024 •

edited

Loading

brandstetterm Aug 28, 2024 •

edited

Loading