-
-
Notifications
You must be signed in to change notification settings - Fork 243
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
Plan layout for floorplan-like pages with system/custom widgets #1100
Conversation
Job #217: Bundle Size — 10.65MB (0%).Changed assets by type (0/7)
|
Thanks @tarag for your work, just know that it's not been unnoticed! There are surely use cases for this type of layout. |
Hi @ghys , now that 3.2 is in progress, could you tell me your concerns regarding this PR? |
Sorry for the delay @tarag I'll do a review this week. |
Apologies for my failure to follow up this month @tarag... For a start, we shouldn't call it "floorplan layout" in my opinion, because:
After thinking long and hard (!) on how to call it, I'd like to suggest "canvas" - I believe it conveys well the fact that you have a frame of sorts you can "draw" widgets on (i.e. position them freely). Second, I think this new layout has a great deal of similarity with the existing "fixed grid" layout type; especially considering the fact that it has a "snap to grid" feature. I also don't quite like having a menu with 2 top-level fixed layout types at the same level as the responsive layout which should remain the default, recommended option for most pages unless you target it specifically for a fixed screen size.
(*for backwards compatibility) Then the initial "big buttons" menu should be redesigned to show the "responsive" type prominently and the "fixed" types with the 2 sub-types as an alternative. Especially since it doesn't look too good on narrow screens:
Last, it's important to have a good design experience even on a phone so first these "toolbar" buttons above the canvas: should look similar to those in the grid layout: That is, the snap-to-grid & configure canvas (and eventual paste if there's a copied widget) options in a dropdown menu - and "Add Item" should be changed to "Add Widget". When on a touch screen, dragging the handles to resize a widget doesn't scroll the page but dragging the widget itself to move it does - this should be easily fixed hopefully: Please let me know if you agree with all the above, looking forward to have this merged! |
@@ -1,6 +1,6 @@ | |||
<template> | |||
<div> | |||
<template v-if="config.layoutType !== 'fixed'"> | |||
<template v-if="config.layoutType === 'responsive'"> |
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.
As stated above it should be:
<template v-if="config.layoutType === 'responsive'"> | |
<template v-if="!config.layoutType || config.layoutType === 'responsive'"> |
since the layout types were introduced with 3.1 and pages created before don't have it and for backwards compatibility should still be treated as responsive types.
@@ -58,6 +58,11 @@ Vue.use(Trend) | |||
// Import Fulscreen Plugin | |||
import fullscreen from 'vue-fullscreen' | |||
Vue.use(fullscreen) | |||
// Import Vue Draggable Resizable | |||
import VueDraggableResizable from 'vue-draggable-resizable' |
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.
According to RelativeCI it's only a ~70kB overhead on the initial JS (or +4%), so while it's not that much, couldn't this be lazy loaded with the oh-plan-layout
(see how the oh-grid-layout
component imports the vue-grid-layout
library)?
a62b0a8
to
82c087a
Compare
I've been quite slow in the response too so no worries. I've made a big mess with the commits too sorry for than, I hope you can figure the changes...
I agree with you on the risk of confusion. Yet, we could also replace the floorplan with this updated proposal as it probably fulfilled all the needs of current floorplan users but provides more (except the zoom but there is the normal plan for that and we could as a zoom feature to my proposal too). For sure in that case a migration should be provided.
I performed this change both in code and in the new file names.
I did not change that, maybe you can propose something now that the code has been updated?
I did change the add widget button. Concerning the Paste and Snap to Grid button, those should really be thought as temporary use edition tools and not really configuration. In fact the snap to grid function does nothing if you toggle it and then not use it to reposition widgets. On the other hand, I can tell you because I have created several of these new canvases, having them rapidly accessible is very convenient when editing layouts. Therefore I just replaced the snap to grid button with a smaller icon based one, I believe it should no longer cause visual issues on small smartphone screens (which are not really the target for this kind of layouts of course). Are you ok with this?
I did fix some of the event handling to avoid spurious text selection when dragging widgets and activation of the widgets themselves in edition mode. It seems to me this problem was fixed also as a consequence. I did also make the change to lazy-load the |
12a3af4
to
157dacc
Compare
I think there's room for both and they can coexist. With the existing "plan" pages it doesn't matter whether you're on a small (phone) or big screen (1080p or more) and you can zoom & pan to get to your controls - the idea was to have icons and labels only and when you click on them, the actual control will appear in a popover -, while the "canvas" layout's strength is to be able to place full-fledged controls, not just icons and labels, on an image background with less emphasis on the "works on all screens" principle. Hence the importance of naming them differently, and as I said the "canvases" can end up being used for more than floorplans - energy flows diagrams are an ideal use case for them.
Yes I'll do it soon.
That's fair, but maybe if possible add some tooltip to that "snap to grid" button because it might not be that obvious what it does from the icons only (even if we see the grid is appearing, it's so dense it might not be understood right away). By the way I noticed that when you choose the type of a new widget the "snap to grid" option is reset, not sure if it's intended but it was unexpected for me: |
I fully agree, that's what I wanted to do but I couldn't find a way to add tooltip to f7 menu-items, and still can't. I'm not familiar enough with f7/vue, and there is no easy
Thanks for the illustration, this has been fixed. |
@ghys: I have no idea how to fix the commit not signed-off, if this is OK for you, I'll create a new PR from scratch. |
If you wish, but you could as well try an interactive rebase (see https://www.internalpointers.com/post/squash-commits-into-one-git) Something like:
|
80850c0
to
dab86ba
Compare
Thanks for the pointers, somehow that did the job. |
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.
Thanks! Here's a new round of review, but I think it's the last, I'll commit a new version of the layout type selection menu after that and we should be good to go.
bundles/org.openhab.ui/web/src/components/widgets/layout/oh-canvas-item.vue
Outdated
Show resolved
Hide resolved
bundles/org.openhab.ui/web/src/components/widgets/layout/oh-canvas-item.vue
Outdated
Show resolved
Hide resolved
bundles/org.openhab.ui/web/src/components/widgets/layout/oh-canvas-item.vue
Outdated
Show resolved
Hide resolved
bundles/org.openhab.ui/web/src/components/widgets/layout/oh-canvas-item.vue
Show resolved
Hide resolved
bundles/org.openhab.ui/web/src/components/widgets/layout/oh-canvas-layout.vue
Outdated
Show resolved
Hide resolved
bundles/org.openhab.ui/web/src/components/widgets/layout/oh-canvas-layout.vue
Outdated
Show resolved
Hide resolved
bundles/org.openhab.ui/web/src/components/widgets/layout/oh-canvas-layout.vue
Outdated
Show resolved
Hide resolved
bundles/org.openhab.ui/web/src/components/widgets/layout/oh-canvas-layout.vue
Outdated
Show resolved
Hide resolved
bundles/org.openhab.ui/web/src/components/widgets/layout/oh-canvas-layout.vue
Outdated
Show resolved
Hide resolved
bundles/org.openhab.ui/web/src/components/widgets/layout/oh-canvas-layout.vue
Outdated
Show resolved
Hide resolved
…ponsive and grid layouts. Supports system and user-defined widgets. Signed-off-by: Gautier Taravella <[email protected]>
Signed-off-by: Gautier Taravella <[email protected]>
Signed-off-by: Yannick Schaus <[email protected]>
Signed-off-by: Yannick Schaus <[email protected]>
Signed-off-by: Yannick Schaus <[email protected]>
Signed-off-by: Yannick Schaus <[email protected]>
Signed-off-by: Yannick Schaus <[email protected]>
Signed-off-by: Yannick Schaus <[email protected]>
@tarag I finished the PR with a change to the initial layout type selection and changed the imports so vue-draggable-resizable and its CSS is not loaded by default. It would be appreciated if you could write up some docs for the new canvas layout: https://github.com/openhab/openhab-docs/blob/main/ui/layout-pages.md Thanks again for your contribution! |
Thanks, great! I'll do that. |
The canvas layout is great. |
I wanted a way to create pages with floorplan layout but interactive (allowing any kind of widgets). It seemed to me the most logical way was to extend the existing layout pages with one additional layout.
That way, the existing floorplan pages can be kept for those using them.