[next] feat(isMobile): use inject/provide #4758
Conversation
Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
raimund-schluessler
added
2. developing
raimund-schluessler
changed the title
raimund-schluessler
added
3. to review
Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
|
I now also migrated |
There was a problem hiding this comment.
I'm not sure this is a good replacement for isMobile.
The main problem: with this solution isMobile only works inside NcContent component. While all the apps should use this component for app's content, sometimes we use components as just UI components, for example, in NcHeaderItem, in dialogs like Password Confirmation dialogs etc.
isMobile is not a NcContent's state and not a "context" state in general, it is a global state (it is not possible that one subtree context is "in mobile" and another subtree context is not in mobile).
Also, it mixes all isMobile logic and state with NcContent component.
We can make it a global reactive state, aka:
// 📁 aka utils/isMobile.js or composables/useIsMobile.js
// Aka private local isMobile in a module-scope
const _isMobile = ref(false)
function handleWindowResize() {
// ...
}
window.addEventListener('resize', handleWindowResize)
handleWindowResize()
// Export readonly proxy to isMobile to save from mutation outside the module
export const isMobile = readonly(_isMobile)For Composition API usage we can add a super simple composable here as well:
// 📁 composables/useIsMobile.js
export function useIsMobile() {
// Just return the value
return isMobile
}For a simple migration, we can also add a mixin. But I'm also fine with just dropping the mixin as we can use composable in Options API as well.
// 📁 mixins/isMobile.js
import { isMobile as isMobileState } from 'path to util'
// Should be marked deprecated
export const isMobile = {
// P.S. We cannot use setup in a mixin by design
// But we can create a computed and return a ref value directly
computed: {
return isMobileState.value
}
}With this approach isMobile is not created in a component reactive scope and there is no removeEventListener. There is always one global (module-scope) reactive variable with one global event listener. It could be not great. We can make it a little bit better by allowing us to use this state only via useIsMobile to count its users, create an event listener only when needed, only once, and clear it when there is no component that uses it. But it won't work in a mixin.
const isMobile = ref(false)
const handleWindowResize = () => {}
let usersCount = 0
export function useIsMixin() {
usersCount += 1
if (usersCount === 1) {
handleWindowResize()
addEventListener()
}
onUnmounted(() => {
usersCount -= 1
if (usersCount === 0) {
removeEventListener()
}
})
return readonly(isMobile)
}And the same with isFullscreen.
| // if the window height is equal to the screen height, | ||
| // we're in full screen mode | ||
| this.isFullscreen = (window.outerHeight === screen.height) |
There was a problem hiding this comment.
After dropping IE11 support, maybe we can use a native event?
https://caniuse.com/?search=fullscreenchange
There was a problem hiding this comment.
Fine with me, but for Safari on iOS we would seem to need a non-standard fallback.
From the vue-components side, I think this is not really an issue. The components that use
I would say all of them only work if they are used together with However, I see the point that one could use the mixin
I don't really think that this is an issue. The implementation in |
But |
I think that would make it more complex and solve the problem that should have been avoided. This is not the purpose of This is an app-level state. In Even with |
This counter is useful only in one edge case, when Without it |
Which is also not a good solution. It creates a new copy of the state with a new event listener for each component that uses the mixin. This is also not how mixins should be used, because this is not a part of a component local state. So I'd backport a new implementation of |
Or we fix it in However, since I don't really understand your solution / approach and am not well familiar with composables and composition API, please feel free to go ahead here (or in |
☑️ Resolves
This PR introduces a couple of changes
NcContentis migrated to vue 3. Vue 3 has a behavior change that it does not replace the element to which the app is mounted, but inserts into it, see https://v3-migration.vuejs.org/breaking-changes/mount-changes.html#mounted-application-does-not-replace-the-element. Hence, the HTML structure will change a bit fromto
isMobileandisFullscreenmixins were changed to use provide/inject. This allows to only use one event listener for theresizeevent, which should be more performant than creating a new one each time the mixins are used. This means that the mixins only work whenNcContentis an ancestor, but since this is required anyway, it should not be an issue (as long as you use nextcloud-vue as intended).