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

Complete rounding by adding selection #4103

Merged
merged 5 commits into from
Aug 1, 2023
Merged

Complete rounding by adding selection #4103

merged 5 commits into from
Aug 1, 2023

Conversation

andydotxyz
Copy link
Member

This is all we can do without rounded shadows I think...

Fixes #3981

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.
  • Public APIs match existing style and have Since: line.

@coveralls
Copy link

coveralls commented Jul 28, 2023 •
edited
Loading

Coverage Status

coverage: 66.335% (+0.003%) from 66.332% when pulling e374b89 on andydotxyz:fix/3981 into 082c2be on fyne-io:develop.

@Jacalz Jacalz self-requested a review July 28, 2023 20:38
Jacalz
Jacalz reviewed Jul 29, 2023
View reviewed changes
Copy link
Member

@Jacalz Jacalz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think we might want to work a little bit more on the spacing for the rounded selection.
For example, the spacing in the Table widget looks a little bit off:
image
It also kind of seems to not be centered in list and tree. Larger gap at the top or bottom?
image

I think it would be nice to do something like Adwaita where the selection has a slight bit of padding on the outside (though I suspect we want less if that works with our design):
image

Jacalz
Jacalz requested changes Jul 29, 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 think you also might want to update the dialog fileitem to use the new selection radius.

@andydotxyz
Copy link
Member Author

I'm not sure I follow which was missed, pretty sure I tested file dialog which was moved to GridWrap?

@Jacalz
Copy link
Member

Jacalz commented Jul 31, 2023

I'm not sure I follow which was missed, pretty sure I tested file dialog which was moved to GridWrap?

File dialog was never moved to GridWrap

@andydotxyz
Copy link
Member Author

andydotxyz commented Jul 31, 2023 •
edited
Loading

I am quite confused about what is going on then.
Here is what I see:

screenshot-20230731T141510

@andydotxyz
Copy link
Member Author

I see on dialog/file.go line 481:

	f.files = container.NewGridWrap

@dweymouth
Copy link
Contributor

@Jacalz is right - that's the GridWrap container, not the GridWrap widget

@andydotxyz
Copy link
Member Author

@Jacalz is right - that's the GridWrap container, not the GridWrap widget

Aha absolutely. Thanks.
But is there more work to be done here, it looks correct to me...

@Jacalz
Copy link
Member

Jacalz commented Jul 31, 2023

What I meant is that I couldn't see any mention in this PR about updating the fileItem to use the new radius. It still uses the input radius and I suspect it should use this section radius?

@andydotxyz
Copy link
Member Author

Aha I understand. I'd forgotten That some selection was using input radius. I will check for others.

@andydotxyz
Copy link
Member Author

That should fix it. I have a WiP move to GridWrap but the selection handling is proving a little complex so I'd like to open it as a different PR

@Jacalz
Copy link
Member

Jacalz commented Aug 1, 2023

No problem. I wasn't suggesting it as part of this one :)

Did you have a look at my comment regarding the alignment?

@andydotxyz
Copy link
Member Author

Did you have a look at my comment regarding the alignment?

I missed them.

Regarding spacing / padding around the rectangles - this has not changed since last release.
On a lowDPI screen as screenshotted the spacers are 1px wide in a 4px space - this will cause what you observed - it has been the same for a couple of releases now.
I don't think we should add more space around curved rectangles than we did around sharp ones.

I don't know exactly what you mean by "a slight bit of padding on the outside", but I am pretty sure that accommodating something like that would potentially mess with the otherwise standard alignment and/or be complicated additions to the theme API.

@Jacalz
Copy link
Member

Jacalz commented Aug 1, 2023

Alright. I'll have a second look at this this eve thing if I can

Jacalz
Jacalz reviewed Aug 1, 2023
View reviewed changes
Copy link
Member

@Jacalz Jacalz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

You are right. This looks great :)
Just one question, are the tabs left intentionally non-rounded?

@andydotxyz
Copy link
Member Author

Yes. We have an underline on tabs. If we round the rectangles those lines will look peculiar won't they?

If you can think of a nice layout I'll look into it but I couldn't find a way that made sense.

@Jacalz
Copy link
Member

Jacalz commented Aug 1, 2023

I assumed that was the case, thanks. Just wanted to be sure. The only thing I can think of is having only the top two corners rounded but that isn't currently possible. I'll approve this :)

@andydotxyz andydotxyz merged commit cfca1a7 into fyne-io:develop Aug 1, 2023
@andydotxyz andydotxyz deleted the fix/3981 branch August 1, 2023 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@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.
4 participants
@andydotxyz @coveralls @Jacalz @dweymouth