Skip to content
This repository has been archived by the owner on Mar 12, 2020. It is now read-only.

[Dev] isPad is an anti-pattern and should be isolated #240

Open
orta opened this issue Aug 16, 2016 · 12 comments
Open

[Dev] isPad is an anti-pattern and should be isolated #240

orta opened this issue Aug 16, 2016 · 12 comments

Comments

@orta
Copy link
Contributor

orta commented Aug 16, 2016

OK, so we obviously have to do a bunch of work with iPad, and most of the time it looks like this:

 const isPad = Dimensions.get('window').width > 700

which is everywhere:

screen shot 2016-08-16 at 9 33 46 am

So, lets try isolate this. I think the simplest thing to do right now is to create a class that exists to just give us that value, so that end user code would look like:

import Device from '../../device'

const isPad = Device.isPad

So that at least we only have that size check in one place, then we can start looking into better ways to understand whether we're in iPad or not. If we want to be able to expand the scope of these view controllers to other OS/platforms we'll need to be able to do different work there.

Alternatives:

  • Can we determine the width of the React view controller at the highest level of the stack, instead of jumping to the device?
  • Can we determine using UIKit's size classes?

I assume you've all looked at this before, but I can also take a fresh look too.

@orta
Copy link
Contributor Author

orta commented Aug 16, 2016

We're also writing hard to understand stylesheets as a part of this problem, for example:

const styles = StyleSheet.create({
  title: {
    margin: isPad ? 40 : 20,
    marginBottom: isPad ? 30 : 20,
    marginTop: isPad ? 50 : 40,
    fontSize: isPad ? 30 : 26,
    alignSelf: 'center',
    textAlign: 'center',

We should look into fully separating the "large" version of the stylesheet and the small one, as they are just JS objects then assign one on top of the other, and using the result should work fine:

var sheet = { 
  title: {
    margin: 20,
    alignSelf: 'center',
    textAlign: 'center',
  }
}
if isPad {
  sheet = Object.assign(sheet, {
    title: {
      margin: 40
    }
  });
}
const styles = StyleSheet.create(sheet)

Or we can make our own functions to generate them: (psuedocoded)

const styles = ARStyleSheet.create({
  title: {
    margin: 20,
    alignSelf: 'center',
    textAlign: 'center',
  },
}, 
large: {
  title: {
    margin: 40
  } 
})

@orta
Copy link
Contributor Author

orta commented Aug 16, 2016

There's also the spread ... operator:

var sheet = { 
  title: {
    margin: 20,
    alignSelf: 'center',
    textAlign: 'center',
  }
}
if isPad {
  sheet = {
    ...sheet
    title: {
      margin: 40
    }
  };
}
const styles = StyleSheet.create(sheet)

@sarahscott
Copy link
Contributor

sarahscott commented Aug 16, 2016

I definitely prefer larger to isPad because eventually we might be building for tablets instead of iPads.

I haven't looked into whether or not we can use UIKit size classes but it feels awkward to do so. I would rather use the RN API for Dimensions in any case.

The conditional stylesheet ideas look good; I think it'll be a matter of stylistic preference there.

@orta
Copy link
Contributor Author

orta commented Aug 16, 2016

The thing with dimensions is that it relies on the window, which we know is now disconnected from the actual size of the view controller that you are rendering in -

skype-per-ipad-split-view-630x467

At least with size classes we're abstracted from that

@orta
Copy link
Contributor Author

orta commented Aug 21, 2016

Found: https://github.com/ayoubdev/react-native-responsive

This is how you would solve this problem in web

@mennenia
Copy link
Contributor

So I remember talking to @alloy about responsive design as a solution.

If I recall properly, his stance was that we still deal with fairly fixed situations and sizes, so it might be overkill (Eloy, please correct as appropriate!).

Personally, I quite like it. I think the native stackview options in iOS 9 were also heading that way, where you can choose for items to be in proper different places on rotation for instance, which was long needed.

What I don't get, @orta, is that from the looks of what you linked, they do still use deviceHeight and Width to determine the layout, which as you rightly mentioned earlier, wouldn't help on split views on iPad right?

Really, the question is, can we determine the size of the view rather than the window/device?

@orta
Copy link
Contributor Author

orta commented Aug 22, 2016

No, but it moves the problem to a single point, where once we figure out how to change it, it's simply a matter of applying it once rather than everywhere.

@mennenia
Copy link
Contributor

Oh of course, single point is great and I didn't dispute that.

@orta
Copy link
Contributor Author

orta commented Aug 22, 2016

We could create a native module whose responsibility is to find which UIViewController the view sits inside, then use that view controller's view's bounds?

@orta
Copy link
Contributor Author

orta commented Aug 27, 2016

Could this come from the React context?

@orta
Copy link
Contributor Author

orta commented May 17, 2017

https://github.com/ctrlplusb/react-sizeme is something looked at in reaction-force -

@sarahscott
Copy link
Contributor

Since this issue, we've started implementing RN's onLayout pattern. However, there are still plenty of places where we use Dimensions.

A great First Task could be to convert instances of the Dimensions pattern to use onLayout instead, and styled-components where appropriate.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants