Mobile: fix side-menu width on tablet devices #6662

Tolu-Mals · 2022-07-12T15:34:56Z

Summary

This pull request fixes the issue of the side-menu becoming too wide on devices with wider screens like tablet devices. It's a part of the Tablet Layout Project

I decided to include the test setup inside the test to make the tests easier to read and understand. Also, future tests might not require the same set up, so it's best to include the setup only the test that requires it.

…package

I added sideMenuWidth state to the root component, then set up an event listener to update the sideMenuWidth state everything the screen width changes. I also set the sideMenuWidth state as a key to the side menu component to make it rerender when the sideMenuWidth state changes.

The former method is deprecated

…tResponsiveValue.ts and getResponsiveValue.test.ts

Array syntax implementation is removed to make the code more self documenting. The first test is modified to check that an exception is thrown when an empty valueMap object is passed to getResponsiveValue, since there's no reason for that.

To reduce boiler plate code and make tests easier to maintain.

Change screenWidthChangeEvent to unsubscribeScreenWidthChangeHandler_ to follow naming convention.

I replaced Dimensions.get('screen') with Dimensions.get('window') because that gives a more concise value that excludes the status bar or navigation bar on an android device.

react-native-side-menu had an issue where it wasn't calculating the side menu's width properly, when a device's width changes , as a result the side menu wasn't rendering consistently with the openMenuOffset values being passed. To fix this, I overrided a method, with a slightly different version. The sidemenu now renders consistently when a device's width cahnges.

The logic for getting the side menu width was being repeated in two parts of the code, so I created a function for that. Also, I made the side menu width increase with each successive breakpoint to take advantage of the screen width to display longer note titles without truncating it.

The sidemenu component rerenders when sideMenuWidth changes without the key.

laurent22 · 2022-07-12T15:59:39Z

Thanks Tolu. You have some conflicts that need to be resolved

Tolu-Mals · 2022-07-24T01:41:59Z

It looks like a dependency is missing! Try running yarn install again and committing changes to yarn.lock.

Error: ➤YN0000: │ root@workspace:.STDERR➤YN0000: [@joplin/app-mobile]: tools/buildInjectedJs.ts(6,45): error TS7016: Could not find a declaration file for module 'fs-extra'. 'D:/a/joplin/joplin/packages/app-mobile/node_modules/fs-extra/lib/index.js' implicitly has an 'any' type.

(A dependency on @types/fs-extra was added in a recently-merged PR)

Thanks. I'll try that.

Tolu-Mals · 2022-07-24T12:13:43Z

It looks like a dependency is missing! Try running yarn install again and committing changes to yarn.lock.

Error: ➤YN0000: │ root@workspace:.STDERR➤YN0000: [@joplin/app-mobile]: tools/buildInjectedJs.ts(6,45): error TS7016: Could not find a declaration file for module 'fs-extra'. 'D:/a/joplin/joplin/packages/app-mobile/node_modules/fs-extra/lib/index.js' implicitly has an 'any' type.

(A dependency on @types/fs-extra was added in a recently-merged PR)

After running yarn install there were no changes in yarn.lock

Tolu-Mals · 2022-07-24T14:47:08Z

A test is now failing due to a linter error in getResponsiveValue.ts:

  41:10  error  Expected '!==' and instead saw '!='  eqeqeq
  45:9   error  Expected '!==' and instead saw '!='  eqeqeq
  49:9   error  Expected '!==' and instead saw '!='  eqeqeq
  57:9   error  Expected '!==' and instead saw '!='  eqeqeq
  65:25  error  Expected '!==' and instead saw '!='  eqeqeq
  69:26  error  Expected '!==' and instead saw '!='  eqeqeq
  73:27  error  Expected '!==' and instead saw '!='  eqeqeq

eslint expects only strict equality to be used. I'll see if there's a way to rewrite the logic without the '!='

Tolu-Mals · 2022-07-25T09:01:37Z

I think this is ready to be merged.

packages/app-mobile/components/getResponsiveValue.ts

Co-authored-by: Henry Heino <[email protected]>

laurent22 · 2022-07-28T13:53:31Z

packages/app-mobile/jest.config.js


-const config = {
- preset: 'ts-jest',
+ testMatch: ['**/*.test.js'],


Why was ts-test removed? I thought we were keeping this?

Wait, actually @personalizedrefrigerator set it up with ts-jest, is that right? In that case, yes please use this for this particular package.

I saw this a bit late. I'll fix that.

I'm currently setting up app-mobile to use ts-jest for testing and I've realized that the preset doesn't need to be 'ts-jest' for ts-jest to be used. The preset needs to 'react-native'. Just leaving this here to avoid any confusion. I followed this guide: Using Typescript With React Native.

I don't know about this, but I think that's going to conflict with what @personalizedrefrigerator is doing? @personalizedrefrigerator will your tests still work with this change?

Currently, tsconfig.json is configured to not create .js files from .test.ts files. As such, unless tsconfig.json is modified, it shouldn't work with my test files.

I changed tsconfig in my recent commit. Your tests are running fine now as well as mine.

I would like some consistency in the tests. Either it's all .js or it's all .ts but not mix and match. I liked the approach of running the .test.ts files directly because it means we don't need source maps.

Is it not possible to get that working with your tests @Tolu-Mals? If there's an issue please explain clearly what it is.

It's possible. I'll do that.

laurent22 · 2022-07-28T13:54:23Z

.gitignore

+packages/app-mobile/components/get-responsive-value.test.js
+packages/app-mobile/components/get-responsive-value.test.js
+packages/app-mobile/components/get-responsive-value.test.js


I don't think that should be here - run yarn run updateIgnored from the root

I don't know why but running updateIgnored from root doesn't remove it from the .gitignore

laurent22 · 2022-07-28T13:55:29Z

packages/app-mobile/components/SideMenu.ts

+type Event = {
+ nativeEvent: {
+ layout: {
+ width: number;
+ height: number;
+ };
+ };
+};


Please remove. We can't have each file define types for third-party libraries. That should maybe be in a separate file shared by all other files. But for now just remove.

laurent22 · 2022-07-28T13:57:58Z

packages/app-mobile/tsconfig.json

-
- // Files that don't need transpilation
- "gulpfile.ts",
- "tools/*.ts",
- "**/*.test.ts",
- "**/*.test.tsx",


Same here, why was that changed?

I'll fix that.

This commit removes support for js test files from jest. This is because only typescript is to be used for testing in app-mobile.

laurent22 · 2022-08-08T14:55:41Z

I think it's good now but please resolve the yarn.lock conflict

laurent22 · 2022-08-08T16:44:40Z

Tests are failing: https://github.com/laurent22/joplin/runs/7729267796?check_suite_focus=true#step:10:4324

Some tests need this to work.

Tolu-Mals · 2022-08-08T17:59:44Z

Tests are failing: https://github.com/laurent22/joplin/runs/7729267796?check_suite_focus=true#step:10:4324

Thank you. I just made a fix, I think they should pass now.

Tolu-Mals · 2022-08-08T18:31:56Z

@laurent22 It's good now.

laurent22 · 2022-08-19T20:53:27Z

Unfortunately still a yarn.lock conflict. A new one this time.

Removed jsdom as the default test environment, it's better for specific files that need it to define it inside the file. Also defined types in the tsconfig.

laurent22 · 2022-08-25T15:59:56Z

Ok I've tested it on simulator and it looks good, thanks Tolu!

Tolu-Mals added 28 commits June 15, 2022 20:02

Set up jest in mobile app

0ac087f

Write and test getResponsiveValue() part 1

fd20ace

Write and test getResponsiveValue() part 2

deed553

Change responsive breakpoints

9d970c8

Set up jest

98450a4

Remove jest set up files

3c4add5

I decided to include the test setup inside the test to make the tests easier to read and understand. Also, future tests might not require the same set up, so it's best to include the setup only the test that requires it.

Write and test getResponsiveValue() part 3

fb4b845

Write and test getResponsiveValue() part 4

113661b

Write and test getResponsiveValue() part 5

f1dc2df

Refactor side-menu.js into Typescript, update react-native-side-menu …

44e5b34

…package

Make side-menu width responsive 2

efce5ca

Change removeEventListener to remove()

609d71c

The former method is deprecated

Rename get-responsive-value.ts and get-responsive-value.test.ts to ge…

81c91c6

…tResponsiveValue.ts and getResponsiveValue.test.ts

Remove getResponsiveValue array syntax tests

15ee067

Remove array syntax code, modify first test

0061dc2

Array syntax implementation is removed to make the code more self documenting. The first test is modified to check that an exception is thrown when an empty valueMap object is passed to getResponsiveValue, since there's no reason for that.

Refactor getResponsiveValue tests

11c46e2

To reduce boiler plate code and make tests easier to maintain.

Add test to CI

81010d6

Rename side-menu.ts into sideMenu.ts

5906839

Change require() to import to support type checking

5a86289

Rename screenWidthChangeEvent

857c825

Change screenWidthChangeEvent to unsubscribeScreenWidthChangeHandler_ to follow naming convention.

Rename get-responsive-value to getResponsiveValue 2

5d71754

Replace 'screen' with 'window'

3a4800c

I replaced Dimensions.get('screen') with Dimensions.get('window') because that gives a more concise value that excludes the status bar or navigation bar on an android device.

Remove console.log()

b883acc

Remove SideMenu Key

8ce930b

The sidemenu component rerenders when sideMenuWidth changes without the key.

Add parameter type

cb38953

Merge branch 'dev' into tl-sidemenu-fix

606adb6

Tolu-Mals added 2 commits July 24, 2022 04:20

0d0bc0b

add gulpfile.js

86fcaa7

Tolu-Mals added 3 commits July 24, 2022 13:56

Add @types/fs-extra

157d1f3

Remove gulpfile.js

920b0a6

9c84caf

Use strict equality to fix eslint error

ea2fed2

personalizedrefrigerator reviewed Jul 25, 2022

View reviewed changes

packages/app-mobile/components/getResponsiveValue.ts Outdated Show resolved Hide resolved

personalizedrefrigerator reviewed Jul 25, 2022

View reviewed changes

packages/app-mobile/components/getResponsiveValue.ts Outdated Show resolved Hide resolved

Tolu-Mals and others added 2 commits July 25, 2022 10:55

Refactor getResponsiveValue.ts

ce49380

Co-authored-by: Henry Heino <[email protected]>

Refactor getResponsiveValue.ts 2

abe4bb8

Co-authored-by: Henry Heino <[email protected]>

laurent22 reviewed Jul 28, 2022

View reviewed changes

Tolu-Mals added 3 commits July 29, 2022 14:38

Configure app-mobile to use ts-jest, update ignored files

45962c7

Remove type definition

3398233

Remove js test file support

e79a0d8

This commit removes support for js test files from jest. This is because only typescript is to be used for testing in app-mobile.

Merge branch 'dev' into tl-sidemenu-fix

1583adc

Use jsdom environment for testing

6084f16

Some tests need this to work.

Tolu-Mals added 3 commits August 20, 2022 10:56

Merge branch 'dev' into tl-sidemenu-fix

a7217b8

fe2ffd1

Update jest and typescript config

60a4a4f

Removed jsdom as the default test environment, it's better for specific files that need it to define it inside the file. Also defined types in the tsconfig.

laurent22 merged commit 8b06cbf into laurent22:dev Aug 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mobile: fix side-menu width on tablet devices #6662

Mobile: fix side-menu width on tablet devices #6662

Tolu-Mals commented Jul 12, 2022

laurent22 commented Jul 12, 2022

Tolu-Mals commented Jul 24, 2022

Tolu-Mals commented Jul 24, 2022

Tolu-Mals commented Jul 24, 2022 •

edited

Loading

Tolu-Mals commented Jul 25, 2022

laurent22 Jul 28, 2022

Tolu-Mals Jul 28, 2022

Tolu-Mals Jul 29, 2022

laurent22 Jul 30, 2022

personalizedrefrigerator Jul 30, 2022

Tolu-Mals Jul 30, 2022

laurent22 Jul 30, 2022

Tolu-Mals Jul 30, 2022

laurent22 Jul 28, 2022

Tolu-Mals Jul 28, 2022

Tolu-Mals Jul 29, 2022

laurent22 Jul 28, 2022

Tolu-Mals Jul 28, 2022

laurent22 Jul 28, 2022

Tolu-Mals Jul 28, 2022

laurent22 commented Aug 8, 2022

laurent22 commented Aug 8, 2022

Tolu-Mals commented Aug 8, 2022

Tolu-Mals commented Aug 8, 2022

laurent22 commented Aug 19, 2022

laurent22 commented Aug 25, 2022

Mobile: fix side-menu width on tablet devices #6662

Mobile: fix side-menu width on tablet devices #6662

Conversation

Tolu-Mals commented Jul 12, 2022

Summary

laurent22 commented Jul 12, 2022

Tolu-Mals commented Jul 24, 2022

Tolu-Mals commented Jul 24, 2022

Tolu-Mals commented Jul 24, 2022 • edited Loading

Tolu-Mals commented Jul 25, 2022

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

Choose a reason for hiding this comment

laurent22 commented Aug 8, 2022

laurent22 commented Aug 8, 2022

Tolu-Mals commented Aug 8, 2022

Tolu-Mals commented Aug 8, 2022

laurent22 commented Aug 19, 2022

laurent22 commented Aug 25, 2022

Tolu-Mals commented Jul 24, 2022 •

edited

Loading