Better detection of device orientation based on browser feature availability #5172
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Prefer to use screen.orientation API first - Then check for window.orientation (e.g. if using Safari browser on a mobile device) - Finally, fall back to using window.innerWidth/window.innerHeight
- Prefer to use hasOwnProperty() over truthy check
- Provides Agent instance to Notebook component - Determine device type and orientation using Agent methods instead of checking against body.classList
Codecov Report
@@ Coverage Diff @@
## master #5172 +/- ##
==========================================
- Coverage 50.15% 49.90% -0.26%
==========================================
Files 548 534 -14
Lines 20101 19555 -546
Branches 1859 1767 -92
==========================================
- Hits 10082 9759 -323
+ Misses 9538 9350 -188
+ Partials 481 446 -35
Continue to review full report at Codecov.
|
akhenry
approved these changes
May 20, 2022
| @@ -52,7 +52,6 @@ export default (agent, document) => { | |||
| if (agent.isMobile()) { | |||
| const mediaQuery = window.matchMedia("(orientation: landscape)"); | |||
| function eventHandler(event) { | |||
| console.log("changed"); | |||
| // address in https://github.com/nasa/openmct/issues/4875 | ||
| // eslint-disable-next-line compat/compat | ||
| const isPortrait = window.screen.orientation.type.includes('portrait'); | ||
| const isPhone = this.agent.isPhone(); |
|
Awesome work with this, thanks @ozyx ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #4875
Describe your changes:
Adds multiple methods of detecting device orientation (Portrait vs. Landscape) based on browser feature availability. Prefer to use
screen.orientationif available, falling back to using the deprecatedwindow.orientation(in case of using Safari mobile browser), and finally falling back to comparingwindow.innerHeightandwindow.innerWidth.Injects an instance of the
Agentutility class into theNotebookVue component in order to directly call these device and orientation classification methods. Previously, theNotebookwould check for specific classes within thedocument.body.classListon re-render in order to determine device type and orientation. However, this leads to a race-condition when changing orientation on a phone, since the Vue component re-renders before theDeviceClassifiercan add/remove the proper classes to thedocument.body.It seems to me that the
Agentutility class might be better as a collection of static utility methods, since it does not necessarily need to maintain state. However, this has broader implications on testing methods and overall architecture, so for this PR I went with the strategy of directly injecting an instance ofAgentas we do in the initialization of theDeviceClassifierplugin. I suppose this helps to decoupleDeviceClassifierandNotebook, so maybe a good strategy? Open to any opinions on this.All Submissions:
Author Checklist
Reviewer Checklist