-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
Conversation
LGTM /cc @vjeux on naming/structuring conventions. ps: |
@tadeuzagallo @vjeux Names are easy to fix. Just lemme know. It's all ViewPort in the code right now (as you can see). |
+1. Need this for sizing things like subviews of a horizontal ScrollView or a |
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 |
@ericvicenti Would it make sense to re-implement what I've got here and add some Listeners like in 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. |
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:
@vjeux , what do you think? Also, we shouldn't refer to |
@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 We'll see how it goes. |
@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; |
There was a problem hiding this comment.
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
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 |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Viewport
instead of ViewPortDimensions
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 |
There are many things to consider for this API.
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. |
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"). |
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. |
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 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 Let me know what you guys think. |
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 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?). |
Agreed. |
Yes I think that both are needed as soon as possible. |
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! |
We need it in core anyway so I'm going to leave it open for now |
@vjeux couldn't core npm install it like react-timer-mixin? |
@ide: This is a possibility |
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 :( |
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 |
Assigning to vjeux since he wants it unassigned and we should try to keep all issues with an internal owner... |
@pjjanak updated the pull request. |
@vjeux Now that a third party module is available, do we still want this? |
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! 😄 |
I think a lot of the benefit of this API is now available by simply adding an |
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
<!-- 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> -->
Basing this off of how
PixelRatio
works. Just forwards toDimensions
and requests the other two properties that have been exposed onDimensions.window
. Also set documentation to on.