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

Added option to hide Container #81

Merged
merged 7 commits into from
Sep 24, 2023

Conversation

kidk
Copy link
Contributor

@kidk kidk commented Jul 5, 2023

I needed a way to hide containers but I couldn't figure out a way to do so.

I plan to update the docs and add an example, but could you check if you like the way it was programmed before I do that.

Thanks a lot!

Example usage

buildingContainer := widget.NewContainer(
		// the container will use a plain color as its background
		widget.ContainerOpts.BackgroundImage(image.NewNineSliceColor(color.NRGBA{0x13, 0x1a, 0x22, 0xff})),

		// the container will use an anchor layout to layout its single child widget
		widget.ContainerOpts.Layout(widget.NewRowLayout(
			widget.RowLayoutOpts.Spacing(10),
			widget.RowLayoutOpts.Padding(widget.Insets{
				Top:    10,
				Bottom: 10,
				Left:   10,
				Right:  10,
			}),
		)),
	)
buildingContainer.Hidden = true

@CLAassistant
Copy link

CLAassistant commented Jul 5, 2023

CLA assistant check
All committers have signed the CLA.

@mcarpenter622
Copy link
Collaborator

mcarpenter622 commented Jul 5, 2023 via email

@kidk
Copy link
Contributor Author

kidk commented Jul 5, 2023

Thanks @mcarpenter622 I knew I wasn't on the right track.

I added the attribute to Widget and adapted the layout render to ignore widgets that are in Hidden state.

This still needs to be tested with all layout types, but wanted to get your thoughts on code.

@mcarpenter622
Copy link
Collaborator

mcarpenter622 commented Jul 5, 2023 via email

@kidk
Copy link
Contributor Author

kidk commented Jul 6, 2023

So I've been playing around a bit and I think there's a corner case I'd like your thoughts on: Do hidden elements take up space? cc @mcarpenter622

Here the Row Layout takes up space, but the buttons do not.
Taking up space

Here the Row Layout doesn't take up space (buttons stay the same).
Not taking up space

In my case I want the row to take up space as it stops the row below from jumping up and down, but I don't want the buttons to do so.

It kinda makes sense to not have Hidden options take up space, which would mean I need to work around the jumping up and down in my project, which I think I can do with an empty container.

@kidk
Copy link
Contributor Author

kidk commented Jul 6, 2023

Alternatively we could add something like HiddenBehaviour with NonBlocking and Blocking

@mcarpenter622
Copy link
Collaborator

mcarpenter622 commented Jul 6, 2023 via email

@kidk
Copy link
Contributor Author

kidk commented Jul 6, 2023

Great idea. I'll code it in and do some testing.

@mcarpenter622 mcarpenter622 linked an issue Jul 13, 2023 that may be closed by this pull request
@kidk
Copy link
Contributor Author

kidk commented Jul 22, 2023

I added an example to play with. There's still odd behavior with RowLayout where the blue box in the example still takes up space although it's set to Visibility_None. I'm not sure how to fix that. Any guidance @mcarpenter622?

I'm not sure about the names also:

Visibility_Show Visibility = iota
Visibility_Hide            // Hide widget, but take up space
Visibility_None            // Hide widget, but don't take up space

When I was using it the difference between Visibility_Hide and Visibility_None was confusing. From the names it's hard to see which is which. What do you think about Visibility_Hide_Blocking and Visibiliy_Hide?

@mcarpenter622
Copy link
Collaborator

@kidk Sorry for taking so long to respond, been dealing with some stuff with work. I am ok with the suggested names. I havent had a chance to check out the Row Layout issue yet.

@kidk
Copy link
Contributor Author

kidk commented Aug 6, 2023

No worries @mcarpenter622 There's no urgency from my side at all, so whenever you have time. 👍

I'll make the suggested changes soon.

@mcarpenter622
Copy link
Collaborator

So I've been testing this tonight and from what I can tell the RowLayout is working as expected. The issue with the blue box in your PR is that it is in a gridLayout. GridLayouts have a column and stretch setting, It doesn't care about what children are in each cell. I changed middleContainer to a RowLayout and added a 3rd element. When I "None" or "Hide' the blue box, the new child behaves as I would expect. All in all this looks like a great change. My only requests are 1) Update the enum names if you'd like, and add a section in the launch.json file for visibility (alphabetized) Otherwise I am happy to merge this!

@mcarpenter622
Copy link
Collaborator

Hey @kidk I really like this PR! Just checking to see if you're still willing to update it? If not I'll make the few changes I mentioned a couple weeks ago and merge it =)

@kidk
Copy link
Contributor Author

kidk commented Aug 24, 2023

I should have some time this weekend to get it into final state. Work has been getting in the way 😄

@mcarpenter622
Copy link
Collaborator

mcarpenter622 commented Aug 24, 2023 via email

@kidk
Copy link
Contributor Author

kidk commented Aug 28, 2023

@mcarpenter622 I've updated the constants and example.

I think it's good for final review

@kidk kidk marked this pull request as ready for review August 28, 2023 08:17
@mcarpenter622 mcarpenter622 merged commit 42a4595 into ebitenui:master Sep 24, 2023
1 of 2 checks passed
@mcarpenter622
Copy link
Collaborator

Thank you for the great PR! Sorry it took me so long to review it. I didnt get an alert when you commented for some reason =(.

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

Successfully merging this pull request may close these issues.

Add Widget Visibility flag
3 participants