Fix responsive #79
Fix responsive #79
Conversation
- The padding were not respected between element in a line and stepping to the next line - We now prefer move the first element in a row to the X padding, to avoid computation - the responsive behavior is to center elements in a row, so it must be OK - We accept now that a non-responsive element is added to the container, it will be resized to the default 100% of the container width - We previously use the window size to compute the reponsive element sizes. That's an error... The reponsivity should be set to the container size. That fixed now.
|
You can delete the old one entirely if you want to. We have no stability guarantees in this repo :) |
This is the case 🙂 I use the layout as I should and I added the container object. |
|
Oh I forget to change the layout.Responsive inside the demo. |
Constants needs initial computation. Prefer using functions
| // Some helpers to define common ratios. | ||
|
|
||
| // FullRatio is the full size of the container. | ||
| func FullRatio() Ratio { return 1.0 } |
There was a problem hiding this comment.
To not store the value. Functions are compiled only if they are used. It's, it seems, a good practice. Maybe I'm wrong.
There was a problem hiding this comment.
Constants are the same https://stackoverflow.com/a/38982889 Most Go I see uses constants
There was a problem hiding this comment.
Yes.
But for example, Fyne returns colors from function calls, like the Padding or like some several values that are "constant". I propose functions to ensure that we can adapt the ratios from possible futur configuration. For example, following themes.
If it's really a problem, I can of course change the code and set it as constants.
There was a problem hiding this comment.
The reason behind theme returning values from a function is that "primary" colour could be respresented by any colour - changed at runtime. However isn't the concept of "TwoThird" always going to be 2/3? If the names were somehow semantic the discussion would track but their names do indicate staticness...
There was a problem hiding this comment.
Lots of nice improvements in there, thanks for submitting this. However I'm not sure that adding more "Responsive" entry points is desirable, is there a way to have fewer package global API with this change? Like if the container had Ali for managing ratios instead of passing in wrapped types?
container/responsive.go
Outdated
| // Responsive(widget.NewLabel("Hello World"), 1, .5), // 100% for small, 50% for others | ||
| // ) | ||
| func NewResponsive(objects ...fyne.CanvasObject) *fyne.Container { | ||
| container := container.New(layout.NewResponsiveLayout()) |
There was a problem hiding this comment.
You can pass the objects in the constructor
| // ctn := NewResponsive() | ||
| // ctn.Add(Responsive(widget.NewLabel("Hello World"), 1, .5)) | ||
| // ctn.Add(Responsive(widget.NewLabel("Hello World"), 1, .5)) | ||
| func Responsive(object fyne.CanvasObject, ratios ...Ratio) fyne.CanvasObject { |
There was a problem hiding this comment.
Hmm, is adding more static functions desirable to handle what is metadata for a single type of container?
It may be a wider design question but if the info cannot be passed in constructor is there a simpler way to handle it that keeps the API entry points to a lower number?
My concern is that "Responsive" could be assumed to work in other places. Like if we accidentally added a "StayLeft" function to describe the position of an item in a border container...
There was a problem hiding this comment.
It's a question I've been asking myself for some time.
NewReponsive returns a *fyne.Container where the Add() method only accepts fyne.CanvasObject, which is perfectly fine.
As a result, I have two possible choices:
- either I allow metadata to be added, as I do with
Reponsive(), but this effectively transforms the objects into "something else with metadata". But the advantage is that I can always add them to another container without affecting the way it works (for example, to test, I change my container to Grid, which doesn't cause any problems). - or I overload
fyne.Containerto return aResponsiveContainerthat have aAdd()method that accepts responsive configuration. But then I'll have two problems:- if I change my container to test another one, the "Add()" method will no longer be respected
- functions that accept
fyne.Containerwill refuse my object
I was thinking of allowing this with NewResponsive() by returning a "config" function signed like this: func(widget, ratios... float32).
So to do it this way:
ctn, configFunc := NewResponsive(label, entry)
configFunc(label, 0.5, 1)
configFunc(entry, 1)
// But how do I add a widget now?
ctn.Add(label2) // This may refresh the view...
configFunc(label2, 0.5) // and here tooIt doesn't look very attractive to me.
There was a problem hiding this comment.
OK, I think we can adapt the NewResponsive function to make this:
label := widget.NewLabel...
entry := widget.NewEntry...
other := widget.NewEntry...
config := []Ratios{
{Object: label, Ratios: []float32{1, 0.5}}, // only the label
}
// use the config (or nil)
ctn := NewReponsive(&config, label, entry) // note that entry is not configured
// adding anthor widget
config.SetRatio(other,0.5, 0.5, 1) // if needed...
ctn.Add(other)That means that I need to keep the Ratios configuration inside the layout, and to remove the Responsive() function + responsive widget.
There was a problem hiding this comment.
I am so sorry this got dropped for such a long time. Are you still happy to refine this to land? I think the discussion about constant named functions returning constant values is an interesting discussion to just complete and see if anything needs to change.
|
I'm of course always open to make modifications. I will take some times to refine my code and my observation. |
Changes Made:
Behavioral Changes:
fyne.Containerutilizing thelayout, replacing the previous improper use.Note: