Roadmap to tablet screen support #100

hyochan · 2020-02-01T07:51:05Z

Description

This is an Initial roadmap to supporting the tablet screen. The goal of supporting the current feature is as follows.

Do not implement the same thing twice other than the design.
Provide independent circumstances so that the code is well-read in each condition (phone or tablet?).
Do not write the test code twice.
- I have ignored the tablet screen from the jest coverage. However, I'd like to include the rendering test for the tablet in the future.

** NOTE **
If you want to support the landscape and portrait mode independently, which we aren't currently considering, that could be inside the tablet or phone file itself. However, I'd wanted to make this clear that phone and tablet rendering sourcecode rely separately upon each file.

Tests

Ignore tablet from coverage currently.
mobile tests still exist and this PR won't affect test codes to be updated.
Add test for DeviceProvider.

Checklist

Before you create this PR confirms that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the [CLA].
Run yarn lint && yarn tsc
Run yarn test or yarn test -u if you need to update snapshot.
I am willing to follow-up on review comments in a timely manner.

codecov · 2020-02-01T07:51:56Z

Codecov Report

Merging #100 into master will decrease coverage by 0.52%.
The diff coverage is 98.16%.

@@            Coverage Diff            @@
##           master    #100      +/-   ##
=========================================
- Coverage   89.23%   88.7%   -0.53%     
=========================================
  Files          44      33      -11     
  Lines        1152    1036     -116     
  Branches      123     116       -7     
=========================================
- Hits         1028     919     -109     
+ Misses         75      69       -6     
+ Partials       49      48       -1

marsinearth · 2020-02-02T09:47:19Z

src/components/screen/SignIn/index.tsx

+import renderMobile from './mobile';
+import renderTablet from './tablet';


Do you have a specific reason why you separated these two seemingly-almost-same files rather than making some branches in styled-components with media queries in one script?

I am also worried about maintenance issues with repeated codes. want to hear how @hyochan think about it

I wanted to make them dependant even we have the duplication.
This is easier for contributors to test out in their env whether they are on tablet or phone.
For step-level code like our project, it isn't a good idea to weave things together.
We'll try to integrate them and make it shorter when we met those stages.

In addition, it looks the same now, but if you see our design in figma, there are much-complicated ones like [Message].

We also don't know yet what's coming next.

I wanted to make them dependant even we have the duplication.
This is easier for contributors to test out in their env whether they are on tablet or phone.
For step-level code like our project, it isn't a good idea to weave things together.
We'll try to integrate them and make it shorter when we met those stages.

That makes sense! it is really good. code sharing sometimes makes it hard to read and that leads other issues when working with different people!

marsinearth · 2020-02-02T09:49:22Z

src/providers/__tests__/DeviceProvider.test.tsx

+});
+
+describe('Interactions', () => {
+  it('should setUser', async () => {


no expect statement...?

Leaving out for your PR 🥇

marsinearth · 2020-02-02T10:00:47Z

src/providers/index.tsx

+        <ApolloProvider client={testClient}>
+          <FriendProvider>
+            <AuthProvider initialAuthUser={initialAuthUser}>
+              <ProfileModalProvider>
+                {children}
+              </ProfileModalProvider>
+            </AuthProvider>
+          </FriendProvider>
+        </ApolloProvider>


damn...these perplexed contexts are out of the form of beauty. I'll definitely work on manipulating Apollo client state management [1][2] to contribute getting rid of them.

@marsinearth do you mean the nested form of providers? this might be helpful

That is a good idea! I hope you can bring up a better code and announce it to us in the meetup.
Also, don't forget to commit expect statement you want to accomplish in the previous review.

godon019

An interesting approach, I would like to try this and then see how it scales as app grows

godon019 · 2020-02-02T11:54:53Z

src/providers/index.tsx

+        <ApolloProvider client={testClient}>
+          <FriendProvider>
+            <AuthProvider initialAuthUser={initialAuthUser}>
+              <ProfileModalProvider>
+                {children}
+              </ProfileModalProvider>
+            </AuthProvider>
+          </FriendProvider>
+        </ApolloProvider>


@marsinearth do you mean the nested form of providers? this might be helpful

- Add [DeviceProvider] to distinguish the device in the first hand. - Disable tablet.tsx from collecting coverage since it`s interaction testing in mostly covered in mobile. - Seperate the ui rendering with mobile & tablet and common utilities are passed by index.tsx.

hyochan added enhancement labels Feb 1, 2020

hyochan added this to the Beta Release milestone Feb 1, 2020

hyochan requested review from godon019 and marsinearth February 1, 2020 08:00

hyochan self-assigned this Feb 1, 2020

marsinearth reviewed Feb 2, 2020

View reviewed changes

godon019 approved these changes Feb 2, 2020

View reviewed changes

hyochan added 5 commits February 4, 2020 01:04

Set default device type to phone

901094c

Support ios tablet splash

fd5407f

Initial roadmap to support tablet view

a4611c2

Remove wrong trailing comma in app.sample.json

dfe8657

hyochan force-pushed the feat/tablet-support branch from 253d739 to dfe8657 Compare February 3, 2020 16:04

marsinearth approved these changes Feb 4, 2020

View reviewed changes

hyochan merged commit d67ed76 into master Feb 4, 2020

hyochan deleted the feat/tablet-support branch February 4, 2020 02:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Roadmap to tablet screen support #100

Roadmap to tablet screen support #100

hyochan commented Feb 1, 2020 •

edited

Loading

codecov bot commented Feb 1, 2020 •

edited

Loading

marsinearth Feb 2, 2020 •

edited

Loading

godon019 Feb 2, 2020

hyochan Feb 2, 2020

hyochan Feb 2, 2020

godon019 Feb 3, 2020

marsinearth Feb 2, 2020

hyochan Feb 2, 2020

marsinearth Feb 2, 2020

godon019 Feb 2, 2020

hyochan Feb 2, 2020

godon019 left a comment

godon019 Feb 2, 2020

		import renderMobile from './mobile';
		import renderTablet from './tablet';

Roadmap to tablet screen support #100

Roadmap to tablet screen support #100

Conversation

hyochan commented Feb 1, 2020 • edited Loading

Description

Tests

Checklist

codecov bot commented Feb 1, 2020 • edited Loading

Codecov Report

marsinearth Feb 2, 2020 • 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

godon019 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hyochan commented Feb 1, 2020 •

edited

Loading

codecov bot commented Feb 1, 2020 •

edited

Loading

marsinearth Feb 2, 2020 •

edited

Loading