Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Viewport] Add utility to get viewport dimensions #418

Closed
wants to merge 7 commits into from

Conversation

pjjanak
Copy link

@pjjanak pjjanak commented Mar 28, 2015

Basing this off of how PixelRatio works. Just forwards to Dimensions and requests the other two properties that have been exposed on Dimensions.window. Also set documentation to on.

@tadeuzagallo
Copy link
Contributor

LGTM

/cc @vjeux on naming/structuring conventions.

ps: Viewport or ViewPort?

@pjjanak
Copy link
Author

pjjanak commented Mar 28, 2015

@tadeuzagallo @vjeux Names are easy to fix. Just lemme know. It's all ViewPort in the code right now (as you can see).

@cleercode
Copy link

+1. Need this for sizing things like subviews of a horizontal ScrollView or a position: absolute views to full width since flex: 1 doesn't cover these cases.

@ericvicenti
Copy link
Contributor

One of the reasons we haven't added this yet is because Viewport size is not so simple. It changes when the call bar comes in, and also when the device rotates to landscape. We need a way to get the current Viewport dimensions, but also a way to subscribe to changes. I'm hesitant to add this until subscriptions are supported, because people will build apps on the bad assumption that Viewport size doesn't change.

Take a look at NetInfo to see how we currently do subscriptions. (We currently use addEventListener, but the interface will be overhauled with observable when we get it in React 0.14. See ParseReact for a preview of this API.)

@pjjanak
Copy link
Author

pjjanak commented Mar 31, 2015

@ericvicenti Would it make sense to re-implement what I've got here and add some Listeners like in NetInfo? I imagine that we could make the emitter let us know if orientation or sizing changes.

Or would that just make more work for the eventual overhaul?

Or, alternatively, would it make sense to make a separate npm package for this? I'm hesitant since it seems like Dimensions is sort of an internal API that you were intentionally leaving unexposed.

@ericvicenti
Copy link
Contributor

We need this in RN core. We have Dimensions internally, but it doesn't support subscriptions, so we haven't exposed it.

Maybe it could look like this:

// get the current viewport size:
var {width, height} = ViewPort.getDimensions();

// subscribe to viewport size changes:
Viewport.addEventListener('dimensionsChange', (newSize) => {
  var {width, height} = newSize;
}

@vjeux , what do you think?

Also, we shouldn't refer to Dimensions because we want to deprecate it. Instead, this module should directly access the lower-level NativeModules.UIManager.Dimensions and RCTDeviceEventEmitter

@pjjanak
Copy link
Author

pjjanak commented Mar 31, 2015

@ericvicenti I think I've got something together that conforms to that API. I've got some more work to do on it tomorrow to make sure everything works, but I could'nt use NativeModules.UIManager.Dimensions as that's just a NSDictionary of values. So I am writing RCTDimensionManager.

We'll see how it goes.

@pjjanak
Copy link
Author

pjjanak commented Apr 1, 2015

@ericvicenti, How's this look for a first pass at this? I'm listening for when the device emits an orientation change and then firing off the new dimensions to any listeners. Figured I'd just add that even for now to see what you thought before digging into all instances where the dimensions might change.

Let me know what you think.

};

Viewport.getDimensions = function(): Object {
return RCTDimension.dimensions;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because you return the static value here without listening to changes, getDimensions will not return an up-to-date dimension after it changes

@ericvicenti
Copy link
Contributor

The JS-facing API looks quite good! Will this catch screen-size changes like the call bar appearing, or just re-orientation?

cc @tadeuzagallo @nicklockwood to get some expert eyes on the ObjC

@pjjanak
Copy link
Author

pjjanak commented Apr 1, 2015

@ericvicenti I swapped out that method for the callback paradigm so you can get up to date information from that method now. And for the time being it's just orientation changes. I'd need to figure out what other events to listen to for sizing updates. But I figured I'd get something going first before I went down that path to make sure I was headed in the right direction.

var DEVICE_DIMENSIONS_EVENT = 'dimensionsDidChange';

/**
* ViewPortDimensions class gives access to the viewport's height and width.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Viewport instead of ViewPortDimensions

@tadeuzagallo
Copy link
Contributor

Yep, looks pretty good, just some nits...

The only things that I'm not sure are the flow annotations, have you seen that @ericvicenti? I don't think we usually explicit use void, and I haven't seen a definition of ChangeEventName, but I'm pretty new to flow, so better leave to you :)

@vjeux
Copy link
Contributor

vjeux commented Apr 2, 2015

There are many things to consider for this API.

  • What is being measured? Your React Native component is embedded inside of an iOS UIView (think of it as iframe on the web). Do we measure this? What happens if you have two instances of React Native sharing the same JavaScript context. How do you get the one that you want. Or, is it measuring the total screen dimensions? What about the bar at the top that comes down? what about the status bar? what about keyboard opening an closing?
  • When is it being measured? On iOS, most of the screen transitions are animated. When the keyboard comes up, it changes the size over time. How do you expose this to the developer? Do you have one callback on every frame? Do you only give the final frame and maybe the time it takes to go there? Do you also give the previous dimensions?

The reason we haven't released an API for this yet is that we need to consider all of those. We don't want to provide an API that's going to be half broken for the interesting use cases.

@nicklockwood
Copy link
Contributor

It should be Viewport. General rule is that single words or hyphenated words shouldn't be camel-cased. Only when two words are joined ("viewport" is a word in it's own right, it isn't a contraction of "view" and "port").

@nicklockwood
Copy link
Contributor

@vjeux

I agree with point one. We should really be broadcasting an event from within RCTRootView whenever the frame changes. I can see how that would be useful - I'm less clear on how the screen itself changing is useful, since the RCTRootView's dimensions might not be directly affected by that, or might not be affected proportionally.

For point two, we should simply send an event every time the size changes. On iOS, the size only changes once during a rotation - the animation between the two sizes in handled internally by the Core Animation render server, so the client code only needs to know the start and end sizes.

@pjjanak
Copy link
Author

pjjanak commented Apr 2, 2015

@vjeux, @nicklockwood

Viewport it is. I had been changing it all to be that anway.

As per what is actual being measured, I am currently using the actual [UIScreen mainScreen].applicationFrame. As I understand it this measures only the area which your application is allowed to take up. I took that to mean the Viewport as it is the space available to your whole app, not just individual components (which would be a whole different beast).

This size does not take into account the notification pane being opened or keyboards or anything of the sort because those don't affect the application's available space. The content still renders beneath them as expected.

This could attach itself to the RCTRootView instead of the applicationFrame, but I don't think that's exactly right. If anything there should probably be two things here. One, which is something like what I'm making here, would only tell you about the space available to your application. Another could then be the space available to components within any particular view. Trying to cram them both into one utility could be difficult and confusing for users.

Let me know what you guys think.

@ide
Copy link
Contributor

ide commented Apr 2, 2015

Chiming in - the viewport dimensions would be useful when showing things like global banners (e.g. when you get a message in Messenger if you're in the app but not in the message's thread). Banners are actually a great use case for controlling multiple RCTRootViews from one JS environment since they can't be implemented purely in React.

That said most of the time I think you want to know the size of the RCTRootView since that's the area that React gets to work with, in the same way that document.body.clientWidth/clientHeight are usually more useful than window.screen.availWidth/availHeight.

In the common case where the RCTRootView takes up the entire screen anyway, the RCTRootView's size is equal to the viewport's. Active phone calls / tethering / map apps that grow the status bar might cause that not always to be true, though, so that's partly why knowing the RCTRootView size is useful. I like @nicklockwood's suggestion to send an event whenever the RCTRootView's frame changes (from layoutSubviews / KVO on layer.bounds?).

@nicklockwood
Copy link
Contributor

Agreed.

@jcgertig
Copy link
Contributor

jcgertig commented Apr 6, 2015

Yes I think that both are needed as soon as possible.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 7, 2015
@pjjanak
Copy link
Author

pjjanak commented Apr 8, 2015

After talking in the IRC chat I've decided to move the contents of this PR into an npm module. As a result I'm going to close this guy out. Thanks for all the feedback guys!

@pjjanak pjjanak closed this Apr 8, 2015
@vjeux
Copy link
Contributor

vjeux commented Apr 8, 2015

We need it in core anyway so I'm going to leave it open for now

@vjeux vjeux reopened this Apr 8, 2015
@ide
Copy link
Contributor

ide commented Apr 8, 2015

@vjeux couldn't core npm install it like react-timer-mixin?

@vjeux
Copy link
Contributor

vjeux commented Apr 8, 2015

@ide: This is a possibility

@brentvatne brentvatne changed the title Add utility to get viewport dimensions [Viewport] Add utility to get viewport dimensions Jun 1, 2015
@vjeux vjeux removed their assignment Jun 1, 2015
@vjeux
Copy link
Contributor

vjeux commented Jun 1, 2015

I'm sorry but the API needs a lot of thoughts as it cannot be removed once added. Right now you can workaround with require('Dimensions') and when I make time to think it through I can really review it. Until then I'll leave it unassigned, sorry :(

@ide
Copy link
Contributor

ide commented Jun 1, 2015

We're in a improving state: react-native-viewport is available on npm for getting the screen dimensions and listening for rotation. For individual views there's now onLayout (although it's too bad composite components don't have it).

@sahrens
Copy link
Contributor

sahrens commented Jun 15, 2015

Assigning to vjeux since he wants it unassigned and we should try to keep all issues with an internal owner...

@vjeux vjeux removed their assignment Oct 16, 2015
@facebook-github-bot
Copy link
Contributor

@pjjanak updated the pull request.

@satya164
Copy link
Contributor

@vjeux Now that a third party module is available, do we still want this?

@brentvatne
Copy link
Collaborator

Sorry @pjjanak, this doesn't seem to be moving. I've created an issue on ProductPains here: https://productpains.com/post/react-native/add-utility-to-get-viewport-dimensions-to-core/ - if it gets upvoted enough then we can dig this back up. Your effort here is much appreciated and will certainly be used if this is needed in the future! 😄

@brentvatne brentvatne closed this Jan 3, 2016
@nicklockwood
Copy link
Contributor

I think a lot of the benefit of this API is now available by simply adding an onLayout handler to your rootView. If your rootView is fullscreen, it will automatically re-layout when the viewport changes. If it isn't, then you probably don't need to care about viewport changes.

facebook-github-bot pushed a commit that referenced this pull request Mar 1, 2017
Summary:
Move configuration to new ```YGConfig``` and pass them down to CalculateLayout. See #418 .

Adds ```YGConfigNew()``` + ```YGConfigFree```, and changed ```YGSetExperimentalFeatureEnabled``` to use the config.

New function for calculation is ```YGNodeCalculateLayoutWithConfig```.
Closes facebook/yoga#432

Reviewed By: astreet

Differential Revision: D4611359

Pulled By: emilsjolander

fbshipit-source-id: a1332f0e1b21cec02129dd021ee57408449e10b0
jfrolich pushed a commit to jfrolich/react-native that referenced this pull request Apr 22, 2020
ayushjainrksh referenced this pull request in MLH-Fellowship/react-native Jul 2, 2020
<!--
Thank you for the PR! Contributors like you keep React Native awesome!

Please see the Contribution Guide for guidelines:

https://github.com/facebook/react-native-website/blob/master/CONTRIBUTING.md

If your PR references an existing issue, please add the issue number below:

#<Issue>
-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet