Skip to content
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

ResponsiveCanvasObject interface + object hiding #59

Conversation

aodhan-domhnaill
Copy link

Two changes,

  • Interface for ResponsiveCanvasObject so that custom types can be used
  • Resizing behavior is handled by the interface method
  • Add hiding behavior so object can be hidden if the window shrinks

First two are self explanatory. The hiding behavior is something that I personally wanted so I can have a "summary/detail" view in a table widget I will am working on.

Let me know if you think I should move the hiding behavior out into another interface.

@andydotxyz
Copy link
Member

I don't think the concept of "ResponsiveCanvasObject" works - only widgets can be extended.
The primitive canvas objects and the container type do not support extending. The code will compile but the objects will not appear (because the graphics driver does not know how to draw them).

@aodhan-domhnaill
Copy link
Author

@andydotxyz I don't know the details, but it works :)

func (rt *ResponsiveTable) AddRow(ele ...fyne.CanvasObject) {
	l := layout.NewResponsiveLayout()
	for i, e := range ele {
		row := layout.Responsive(
			e,
			rt.rowConstants[i]*rt.sizeTotals[0],
			rt.rowConstants[i]*rt.sizeTotals[1],
			rt.rowConstants[i]*rt.sizeTotals[2],
			rt.rowConstants[i]*rt.sizeTotals[3],
		)
		if i > 1 {
			row.Hidable(true)
		}
		l.Add(row)
	}
	rt.tableContainer.Add(l)
}

The behavior is what I would expect too. The details in my table row hide when I shrink the window

@aodhan-domhnaill
Copy link
Author

By the way, I'm building widgets and layouts with the goal to get to https://react-bootstrap-table.github.io/react-bootstrap-table2/. I've been breaking them into separate PRs as I slowly build them out.

If you have a different approach to managing PRs, let me know.

@andydotxyz
Copy link
Member

@andydotxyz I don't know the details, but it works :)


func (rt *ResponsiveTable) AddRow(ele ...fyne.CanvasObject) {

	l := layout.NewResponsiveLayout()

	for i, e := range ele {

		row := layout.Responsive(

			e,

			rt.rowConstants[i]*rt.sizeTotals[0],

			rt.rowConstants[i]*rt.sizeTotals[1],

			rt.rowConstants[i]*rt.sizeTotals[2],

			rt.rowConstants[i]*rt.sizeTotals[3],

		)

		if i > 1 {

			row.Hidable(true)

		}

		l.Add(row)

	}

	rt.tableContainer.Add(l)

}

The behavior is what I would expect too. The details in my table row hide when I shrink the window

I'm not sure that the code sample shows usage of the ResponsiveCanvasObject that I was unsure of?

@aodhan-domhnaill
Copy link
Author

Changed it to extending Widget. Maybe that's better

@andydotxyz
Copy link
Member

Thanks.

What I am curious about is whether this has to be tied to window size? It breaks the container model we use everywhere else.
Can't it just use the container size and conform with the widget/layout contract without the need for connecting to the current window and assuming it has maths based on the whole space?

@aodhan-domhnaill
Copy link
Author

https://github.com/fyne-io/fyne-x/blob/master/layout/responsive.go#L112

It's in the existing code. I didn't like it either, but didn't want to break your existing functionality.

@andydotxyz
Copy link
Member

https://github.com/fyne-io/fyne-x/blob/master/layout/responsive.go#L112

It's in the existing code. I didn't like it either, but didn't want to break your existing functionality.

OK, I guess that's a fair point, but it was an internal detail before, but with this interface method you force it into the public API:

HandleResize(newPos fyne.Position, windowSize, containerSize fyne.Size)

@metal3d
Copy link
Contributor

metal3d commented Nov 13, 2023

Can you please take a look on #79 PR @aodhan-domhnaill? I made a few fixes that makes the Responsive Layouts a bit more flexible.

It's now a container with Layout, and I already tried with custom widgets and container injections. It must fix what you mean here.

Thanks a lot for your tests and help.

Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why this change introduces so many new API. Also it seems to move the container layout logic into widgets which isn't how containers generally work.

Perhaps @metal3d can comment on the general idea of hiding at thresholds and how it fits into responsive overall, but this implementation feels like it may not be the best way?

@andydotxyz
Copy link
Member

Can we close this in favour of #79 my @metal3d ?

@aodhan-domhnaill
Copy link
Author

Closing in favor of #79

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants