Support room-defined widget layouts #5553
Conversation
Ideally the settings store returns the right type, but for now we can feed it the type we need/expect.
Note that this ditches all previously set width values, however this is probably acceptable for now. Trying to remain backwards compatible gets tricky on top of already tricky code, and the impact of Element forgetting widths is not as severe as forgetting which widgets were/are pinned.
Much like widget widths, it is acceptable for us to forget what everyone's height was previously at.
| without an `index`) are resolved by comparing widget IDs. A maximum of 3 widgets can be in the top | ||
| container - any which exceed this will be ignored. Smaller numbers represent leftmost widgets. | ||
|
|
||
| The `width` is relative width within the container in percentage points. This will be clamped to a |
There was a problem hiding this comment.
I wonder if a system like the flex CSS property would be better, where the numbers are just "ratios" rather than percentages? So, if widget 1 has "width: 1" and widget 2 has "width: 2", it means widget 2 is twice as wide as widget 1. Since there are two widgets in this example, it means widget 1 will get 1/3 of available space, while widget 2 will get 2/3.
Maybe it falls apart though because we already allow dragging to adjust layout to arbitrary points, which would encode some quite strange ratios... 😅 Anyway, seemed worth consideration at least.
There was a problem hiding this comment.
Is be worried about values getting obscenely large or out of hand with ratios - we can't store floats.
Percentages also feel somewhat more predictable, imo.
There was a problem hiding this comment.
Okay, I think I just find the percentages distasteful in some way, like they are too low level or something, but I don't think it's worth holding up progress for that, so feel free to continue as you like.
There was a problem hiding this comment.
Aside from feelings about percentages vs other units of measure, there's also time pressure, external requirements, etc pushing this into needing to be percentages :(
We can revist this one day - it is deliberately in our namespace to let us make changes as needed.
| @@ -62,13 +63,13 @@ export default class AppsDrawer extends React.Component { | |||
|
|
|||
| componentDidMount() { | |||
| ScalarMessaging.startListening(); | |||
| WidgetStore.instance.on(this.props.room.roomId, this._updateApps); | |||
| WidgetLayoutStore.instance.on(WidgetLayoutStore.emissionForRoom(this.props.room), this._updateApps); | |||
There was a problem hiding this comment.
If this component can receive a new room through a props change, then you'll need to handle swapping this listener off the old room and onto the new room.
There was a problem hiding this comment.
Not sure I understand - this isn't anything different than before?
There was a problem hiding this comment.
Ah, I didn't really look at the previous line here... I'm saying I would expect componentDidUpdate to swap listeners to the new room ID if it's possible it can change. Since you're right it would have been an issue before, I guess it means we don't actually change rooms after the first mount here.
Co-authored-by: David Baker <[email protected]>
| without an `index`) are resolved by comparing widget IDs. A maximum of 3 widgets can be in the top | ||
| container - any which exceed this will be ignored. Smaller numbers represent leftmost widgets. | ||
|
|
||
| The `width` is relative width within the container in percentage points. This will be clamped to a |
There was a problem hiding this comment.
Okay, I think I just find the percentages distasteful in some way, like they are too low level or something, but I don't think it's worth holding up progress for that, so feel free to continue as you like.
|
@jryans this should have actual math now (don't really know why that was so difficult to think of at the start...) and the |
Co-authored-by: J. Ryan Stinnett <[email protected]>
|
@jryans @turt2live Thank you very much for this much needed change, back to how widgets worked some time ago. One more necessary improvement to Element (Web/Desktop) would be to graphically set which power level is allowed to save the room layout in |
Fixes element-hq/element-web#15267
This is required for FOSDEM.
Note to reviewer: It may be easier to review this commit by commit. The rough history is I wrote all the layout parsing code first then wired up the individual parts, so the layout code got bug fixes in later commits.
This has already passed through design/product review. Further iterations are planned, but this is good enough for now.
There are built-in docs to this PR, but the idea is that by sending a state event into the room then room operators can set a layout that all users see. This notably does not include other requirements of this line of sponsored work such as automatically maximizing widgets (not a concept we have, yet) and locking the layout. These are deferred until later availability.
We aren't totally sold on the placement/behaviour of the copy-my-layout button, but we are hoping to dogfood it and see how it goes.
Relevant screenshots (everything else is an under-the-hood change):
