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.

Sign up for GitHub

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

Jump to bottom

Add GridWrap widget #56

Merged
merged 6 commits into from
May 24, 2023
Merged

Add GridWrap widget #56

merged 6 commits into from
May 24, 2023

Conversation

dweymouth
Copy link
Contributor

@dweymouth dweymouth commented Apr 2, 2023 •
edited

This is a port from fyne to fyne-x of the GridWrapList component I made for Supersonic used for the album grid view. I stripped out the use of private APIs (internal/widget/scroll, BaseWidget.super()) and copied others (syncPool).

@dweymouth dweymouth marked this pull request as draft April 2, 2023 19:25
Jacalz
Jacalz requested changes Apr 4, 2023
View reviewed changes
Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

Well done. This is looking great. I just have a few comments inline about making the code more readable in some places.

We talked a bit on Slack about changing the name to something like GridWrap. I think the List part makes it a bit unclear.

widget/gridwraplist.go Outdated Show resolved Hide resolved
widget/gridwraplist.go Outdated Show resolved Hide resolved
widget/gridwraplist.go Outdated Show resolved Hide resolved
widget/gridwraplist.go Outdated Show resolved Hide resolved
@dweymouth dweymouth requested a review from Jacalz April 9, 2023 15:52
@dweymouth dweymouth marked this pull request as ready for review April 9, 2023 15:53
@dweymouth dweymouth changed the title Add GridWrapList widget Add GridWrap widget Apr 10, 2023
Jacalz
Jacalz requested changes Apr 11, 2023
View reviewed changes
Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

Well done. I mostly just had one small comment in line. Would you mind adding a few simple tests? With that done, I think this should be ready to land.

widget/gridwrap.go Outdated Show resolved Hide resolved
@Jacalz Jacalz requested a review from andydotxyz April 11, 2023 17:49
@Jacalz Jacalz mentioned this pull request Apr 15, 2023
Merged
3 tasks
dweymouth and others added 2 commits April 30, 2023 13:15
@dweymouth @Jacalz
don't pass unnecessary BaseWidget{}
f022b27 
Co-authored-by: Jacob Alzén <jacalz@tutanota.com>
@dweymouth
add some unit tests
00360bf
@dweymouth
Copy link
Contributor Author

Well done. I mostly just had one small comment in line. Would you mind adding a few simple tests? With that done, I think this should be ready to land.

Done!

@dweymouth dweymouth requested a review from Jacalz May 6, 2023 16:45
@Jacalz

This comment was marked as outdated.

Sign in to view
@Jacalz
Copy link
Member

Jacalz commented May 6, 2023

Ignore my previous comment. I had forgotten a refresh call :/

Jacalz
Jacalz requested changes May 6, 2023
View reviewed changes
Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

I just have one minor comment inline. This is looking very good :)

widget/gridwrap_test.go Outdated Show resolved Hide resolved
@dweymouth @Jacalz
Update widget/gridwrap_test.go
e5eb5a7 
Co-authored-by: Jacob Alzén <jacalz@tutanota.com>
Jacalz
Jacalz approved these changes May 6, 2023
View reviewed changes
Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

Thanks. I'm very happy with this. Well done!

Perhaps @andydotxyz or @Bluebugs can review before merging?

andydotxyz
andydotxyz requested changes May 23, 2023
View reviewed changes
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.

This looks really great, thanks.
However I'm not sure about the data binding aspect.
When you attach a DataList it is a list of binding items, it will be triggered if the list changes in dimension, but any child item can be updated when that data changes.
In this implementation those child changes will not be reflected in the UI as the item is not bound, nor is it refreshed on a child change.
I'd recommend removing the binding code for now, landing this, and then working on that separately...

andydotxyz
andydotxyz approved these changes May 23, 2023
View reviewed changes
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.

Apologies, it seems I may have not read the data item code correctly. Looks good.
Happy to proceed if @Jacalz agrees the data binding code is cool to land together with the main API.

@Jacalz
Copy link
Member

Jacalz commented May 24, 2023

I think it is fine. It looks to be the same as what we already have in the List widget.

@Jacalz Jacalz merged commit 8538f64 into fyne-io:master May 24, 2023
7 checks passed
@Jacalz
Copy link
Member

Jacalz commented May 24, 2023

Thanks for your work on this @dweymouth. We appreciate it. This widget is going to be very useful :)

Jacalz added a commit to Jacalz/fyne that referenced this pull request Jun 11, 2023
@Jacalz @dweymouth
widget: Add the GridWrap collection widget from @dweymouth
5ef0c25 
Initial implementation was by @dweymouth over at fyne-io/fyne-x#56. This PR includes that work plus selection support and some of the improvements that I have done (see fyne-io/fyne-x#61 and https://github.com/fyne-io/fyne-x/compare/master...Jacalz:gridwrap-selection?expand=1).

This was done as a PR directly to Fyne and not fyne-x as the `.super()` call from `widget.BaseWidget` wasn't public and not using it made the selection work bad. All of the changes in this PR compared to the widget in fyne-x can be found here: https://github.com/fyne-io/fyne-x/compare/master...Jacalz:gridwrap-selection?expand=1

Co-authored-by: Drew Weymouth <dweymouth@gmail.com>
@Jacalz Jacalz mentioned this pull request Jun 11, 2023
Merged
4 tasks
adamantike added a commit to adamantike/supersonic that referenced this pull request Jun 15, 2023
@adamantike
Use GridWrap from fyne-x
b3353a0 
With fyne-io/fyne-x#56 merged, the local
implementation can be removed to rely on the upstream version.
@adamantike adamantike mentioned this pull request Jun 15, 2023
Merged
dweymouth pushed a commit to dweymouth/supersonic that referenced this pull request Jun 15, 2023
@adamantike @dweymouth
Use GridWrap from fyne-x
7b15fe9 
With fyne-io/fyne-x#56 merged, the local
implementation can be removed to rely on the upstream version.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andydotxyz andydotxyz andydotxyz approved these changes

@Jacalz Jacalz Jacalz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dweymouth @Jacalz @andydotxyz