Feat: onboarding create join project #53

cimigree · 2024-12-09T19:57:23Z

related to #42

Description

Moves some files around
Adds bigger icons
Adds components for the repeated background for the Onboarding Screens
Updates existing screens to use the new OnboardingScreenLayout component
Updates the Onboarding Top Menu so a user cannot bypass the necessary steps
Updates the first Onboarding screen to be more responsive and styled better
Adds the Create project Screen
Adds the Join project screen

Notes

Many of the changed files are just moving files into folders and updating the imports and also updating the size of some icons
I added the Toggle for importing a config but I wasn't sure if that was actually part of the design or if that is done differently on desktop so I didn't add any logic for it
I was not sure when the Join Project screen should show up... If the logic was the same as on mobile (Like what is the idea? Someone will get to the Create or Join Project screen and someone else will be right there and then the invite will be sent?) The discovery will have to be there, right?
I was thinking we could add the logic later and just start with the UI/ screens for now

so you can get to the screen to see it by changing the navigation in the CreateJoinProjectScreen
from:
onClick={() => navigate({ to: '/Onboarding/CreateProjectScreen' })}
to:
onClick={() => navigate({ to: '/Onboarding/JoinProjectScreen' })}

…ice name exists.

… screen. Makes create project screen.

…sue.

… skip steps or go back.

ErikSin · 2024-12-09T22:40:54Z

I think we can simplify the onboarding screen and make it more testable and maintainable by exposing the children at the top level and avoiding such deep nesting.

I would suggest:

// this screen now controls whether the onBack can be pressed, and it does not have to be determined in OnboardingTopMenu
// Also the screen content can be fully determined by the parent. No need to pass props to the OnboardingScreenLayout and conditionally render things based on what screen it is
const SomeOnboardingScreen = () => {

   function onBackPress(){
         navigate(to next screen)
    }
    return(
        <OnboardingScreenLayout
            topMenu={<OnboardingTopMenu onBackPress={ionBackPress) currentStep=}/>}
        >
          {...screen content here}
        </OnboardingScreenLayout>
    )
}

const OnboardingScreenLayout = ({OnboardingTopMenu: ReactNode, children:ReactNode}) => {

    return(
        <Container>
			<OnboardingTopMenu/>
			<ContentWrapper>
				<ContentBox>
                    {children}
                </ContentBox>
            </ContentWrapper>
        </Container>
    )
}

const OnboardingTopMenu = ({onBackPress?, currentStep}) => {

    return (
        <MenuContainer>
            <GoBackButton disabled={!onBackPress} onPress={onBackPress}>
            <Steps>
              {[1, 2, 3].map(step => <Step active={currentStep === step}/>)}
            </Steps>
        </MenuContainer>
    )
}

This suggested architecture does make the component slightly less reusable as It is simply a layout wrapper and the parent determines the content completely.

But any shared styling can still be shared, just pass it around to the children not encapsulated in the OnboardingScreenLayout

The first major thing that this does is it allows the OnboardingTopMenu to be completely agnostic and not reliant on the nav state. In the future if we change the steps, we do not need to refactor the OnboardingTopMenu to account for the changes in navigation. Since the parent just controls this we can simply add or take away a screen with no refactoring.

Secondly we can test the screens in isolation. The OnboardingScreenLayout is solely for stying, so there is no logic to be tested. Previously if we wanted to test the < input /> in the Device name screen, and how it interacted with the button, we would have to test the DeviceNamingSreen, and then test the button rendered inside the OnboardingScreenLayout

src/renderer/src/components/Onboarding/OnboardingTopMenu.tsx

ErikSin · 2024-12-09T22:54:38Z

src/renderer/src/routes/Onboarding/CreateProjectScreen.tsx

+		const value = event.target.value
+		const graphemeCount = countGraphemes(value.trim())
+		const byteLength = getUtf8ByteLength(value.trim())
+		let localError = false
+
+		if (
+			graphemeCount > PROJECT_NAME_MAX_LENGTH_GRAPHEMES ||
+			byteLength > PROJECT_NAME_MAX_BYTES ||
+			value.trim().length === 0
+		) {
+			localError = true
+		} else {
+			setProjectName(value)
+		}
+		setError(localError)


can we write a test for this?

we could write a pure function that does not deal with react state

Something like:

const handleChange = (event: ChangeEvent<HTMLInputElement>) =>{ const value = event.target.value const localError:boolean= checkForError(value) if (!localError) setProjectName(value) if (localError !== error) setError(localError) }

then we could write a test for checkForError which is not reliant on react state, and can be a pure function

I think I did this. I tried to anyway.

… logic out of naming screens. Fixes the onboarding top menu.

cimigree · 2024-12-10T23:51:25Z

@ErikSin, There is still logic that needs to be added, but I think the component and screen structure might be closer to what you had in mind. I didn't make shareable components for the buttons or anything. Hope that is ok, but let me know if I should do that too.

ErikSin

Last things, can yo write tests for your function. Vitest should now be setup!

ErikSin · 2024-12-11T22:24:14Z

src/renderer/src/routes/Onboarding/PrivacyPolicyScreen.tsx

+			<OnboardingTopMenu
+				currentStep={1}
+				onBackPress={() => navigate({ to: '/Onboarding/DataPrivacy' })}
+			/>
 			<ContentBox>
 				<PrivacyPolicy />


Why is the privacy policy a nested component. It basically a container inside a container. it just seems like an unnecessary nesting

I did that in the same way I did it on mobile because the Privacy Policy is used in a separate place within the settings and is almost exactly the same so that way it can be reused as a component there.

ErikSin · 2024-12-11T22:27:19Z

src/renderer/src/routes/Onboarding/DeviceNamingScreen.tsx

+		const localError: boolean = checkForError(
+			value.trim(),
+			DEVICE_NAME_MAX_LENGTH_GRAPHEMES,
+		)
+		setDeviceName(value)
+		if (localError !== inputError) {
+			setInputError(localError)


if there is an error, do we want to set the name? Or should we just set the device name when no error

I am doing that because when there is a blank there (someone erases all of the letters), there is an error and we need to set the device name to a blank. If I don't set the device name in that case a user can never erase that last letter.

ErikSin · 2024-12-11T22:33:38Z

src/renderer/src/routes/Onboarding/CreateProjectScreen.tsx

+				<div
+					style={{
+						width: '100%',
+						maxWidth: 400,
+						margin: '0 auto',
+						textAlign: 'center',
+					}}
+				>
+					<div
+						style={{
+							display: 'flex',
+							justifyContent: 'space-between',
+							alignItems: 'center',
+							gap: 12,
+							padding: '12px 0',
+							cursor: 'pointer',
+						}}
+						onClick={() => setAdvancedSettingOpen(!advancedSettingOpen)}
+					>
+						<Text bold style={{ fontSize: '1.125rem' }}>
+							{formatMessage(m.advancedProjectSettings)}
+						</Text>
+						{advancedSettingOpen ? <ChevronDown /> : <ChevronUp />}
+					</div>


I think we should us an accordian here. Accordian would give the animation, and doesn't require us to use css to hide

Sure. Done.

ErikSin · 2024-12-11T22:39:33Z

src/renderer/src/routes/Onboarding/CreateProjectScreen.tsx

+		<OnboardingTopMenu
+			currentStep={3}
+			onBackPress={() =>
+				navigate({ to: '/Onboarding/CreateJoinProjectScreen' })
+			}
+		/>


we should also disable the back button when setProjectNameMutation.isPending

Sounds good. Done.

ErikSin · 2024-12-11T22:46:01Z

src/renderer/src/components/Onboarding/onboardingLogic.ts

can you write test for these functions? Vitest was just integrated into main, so you can rebase and just start wrtiting test. We are going to colocate are test files (which I will document). So if you can create a onboardingLogic.test.ts file and test these function there

Done! That was fun! Much more fun than E2E tests!

…ding.

ErikSin

Great job and nice tests! Just one small comment about uncontrolled vs controlled state. But otherwise good to merge

ErikSin · 2024-12-12T17:13:42Z

src/renderer/src/routes/Onboarding/CreateProjectScreen.tsx

+						expanded={advancedSettingOpen}
+						onChange={(_, expanded) => setAdvancedSettingOpen(expanded)}


One small thing. Lets make this uncontrolled component. In otherwords, dont pass in the expanded and onChange props, and let it manage its own state. For maintainability, the less state the parent has to control the better.

cimigree added 14 commits December 3, 2024 17:49

Sets up basics for device naming screen. Still needs api call to work.

9d4662b

Removes console logs.

1ebb36c

Adds the next screen create join project screen to check that the dev…

0ed4682

…ice name exists.

Adds segmenter count and byte count to limit device name length.

45e02b5

Makes reusable onboarding layout component. Makes create join project…

5441dda

… screen. Makes create project screen.

Merges with main for device naming grapheme count logic.

b3747c2

Updates grapheme count methods

8b2414b

Moves files into various folders and updates imports. Fixes height is…

527f4c8

…sue.

Adjustments to device naming screen.

087c785

Fixes height of screens. Finishes basic UI for create a project screen.

5465d5f

Allows button to take className prop. Sets up top menu so user cannot…

2e0ffca

… skip steps or go back.

Adds join project screen.

856bf01

Adds bigger icons

35e5687

Fixes styling and responsiveness of first screen.

ce4e810

cimigree requested a review from ErikSin December 9, 2024 20:08

ErikSin requested changes Dec 9, 2024

View reviewed changes

Makes the layout and menu component just styles and less logic. Moves…

586d2d3

… logic out of naming screens. Fixes the onboarding top menu.

cimigree requested a review from ErikSin December 10, 2024 23:51

CHanges spacing to 12 and 20 to reflect design decision.

e99e409

cimigree mentioned this pull request Dec 11, 2024

feat: import config #55

Merged

ErikSin requested changes Dec 11, 2024

View reviewed changes

cimigree added 3 commits December 12, 2024 09:41

Merges from main.

159ff26

Adds MUI accordian. Disables back button when project creation is pen…

c1e4a3c

…ding.

Adds tests for onboarding logic.

fedae31

cimigree requested a review from ErikSin December 12, 2024 16:46

ErikSin approved these changes Dec 12, 2024

View reviewed changes

Removes parent state from accordion.

de1fcff

cimigree merged commit 45c576c into main Dec 12, 2024
2 checks passed

cimigree deleted the feat/onboarding-create-join-project branch December 12, 2024 20:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: onboarding create join project #53

Feat: onboarding create join project #53

cimigree commented Dec 9, 2024 •

edited

Loading

ErikSin commented Dec 9, 2024 •

edited

Loading

ErikSin Dec 9, 2024

cimigree Dec 10, 2024

cimigree commented Dec 10, 2024 •

edited

Loading

ErikSin left a comment

ErikSin Dec 11, 2024

cimigree Dec 12, 2024

ErikSin Dec 11, 2024

cimigree Dec 12, 2024

ErikSin Dec 11, 2024

cimigree Dec 12, 2024

ErikSin Dec 11, 2024

cimigree Dec 12, 2024

ErikSin Dec 11, 2024

cimigree Dec 12, 2024

ErikSin left a comment

ErikSin Dec 12, 2024

		expanded={advancedSettingOpen}
		onChange={(_, expanded) => setAdvancedSettingOpen(expanded)}

Feat: onboarding create join project #53

Feat: onboarding create join project #53

Conversation

cimigree commented Dec 9, 2024 • edited Loading

Description

Notes

ErikSin commented Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cimigree commented Dec 10, 2024 • edited Loading

ErikSin left a 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

ErikSin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cimigree commented Dec 9, 2024 •

edited

Loading

ErikSin commented Dec 9, 2024 •

edited

Loading

cimigree commented Dec 10, 2024 •

edited

Loading