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

Maintenance result alert's width too large #270

Closed
2 tasks done
hubertdej opened this issue Mar 2, 2024 · 12 comments · Fixed by #271
Closed
2 tasks done

Maintenance result alert's width too large #270

hubertdej opened this issue Mar 2, 2024 · 12 comments · Fixed by #271
Assignees
Labels
Bug Something isn't working

Comments

@hubertdej
Copy link

Bug Report

Mandatory Information

If you have a large number of unupdated packages, the alert that is shown after the maintenance check doesn't wrap text and is therefore too wide. It doesn't fit on the screen.

To Reproduce

  1. Have a considerable amount of unupdated packages.
  2. Brew Maintenance... -> Tick "Purge Homebrew cache" -> Start

Expected Behavior
Alert wraps text.

Screenshots
image

System and Cork Information:

  • macOS version: Sonoma 14.0
  • Cork Version: 1.3.4
  • Did you compile Cork yourself?: yes

Checklist

  • I filled everything out under Mandatory Information
  • This bug affects the UI and I have included a screenshot
@hubertdej hubertdej added the Bug Something isn't working label Mar 2, 2024
@rishatl
Copy link

rishatl commented Mar 4, 2024

Hey, I'll take it upon myself, make me assignees, pls

@rishatl
Copy link

rishatl commented Mar 4, 2024

@buresdv, check mr👆🏻pls

@buresdv
Copy link
Owner

buresdv commented Mar 8, 2024

Fixed by #271

@buresdv buresdv closed this as completed Mar 8, 2024
@buresdv
Copy link
Owner

buresdv commented Mar 10, 2024

Reopening due to padding bug.

@rishatl when there's a lot of content, the top and bottom padding breaks in your implementation (which is now on the branch fixed-long-lists):

Snímek obrazovky 2024-03-11 v 0 02 54

Would you mind having a look at it?

@rishatl
Copy link

rishatl commented Mar 11, 2024

@buresdv Removing fixedSize solved this problem

@buresdv
Copy link
Owner

buresdv commented Mar 11, 2024

@rishatl that does fix the padding, but now, most of the text is missing:
Snímek obrazovky 2024-03-11 v 19 02 17

Also, you can expand the sheet to a ludicrous size, which looks bad 😅
Snímek obrazovky 2024-03-11 v 19 02 22

I suppose the fixedSize() has to be somewhere. Do you have any other idea?

@rishatl
Copy link

rishatl commented Mar 12, 2024

@buresdv fixedSize() prevents the view from being expanded, and the text can be of different sizes. I think shortened text is the best solution. Otherwise you will have to bind to constant frame values, which will lead to the same situation of text moving outside the view.

@buresdv
Copy link
Owner

buresdv commented Mar 12, 2024

The text was supposed to be shortened, but that was causing crashes (#173). Would it be possible to shorted the text, allow the sheet to expand, but forbid it from expanding indefinitely?

@rishatl
Copy link

rishatl commented Mar 13, 2024

I propose this solution - a limit on the maximum size with the ability to expand only vertically if there is a lot of text.

            .frame(maxWidth: 800, maxHeight: 800)
        }
        .padding()
        .fixedSize(horizontal: true, vertical: false)

@buresdv
Copy link
Owner

buresdv commented Mar 13, 2024

That is definitely an interesting solution, but it still suffers from the problem of the sheet being expandable to too large of a size:
Snímek obrazovky 2024-03-13 v 15 51 54

Ideally, the text that lists all the packages would expand as needed (with some sort of a layout priority), and then the rest of the sheet would be laid out. But frames in SwiftUI are a huge mystery to me, so I don't really know how to do that

@buresdv
Copy link
Owner

buresdv commented Mar 13, 2024

I think I fixed the underlying problem described in #173. Could you try this branch? https://github.com/buresdv/Cork/tree/fixed-maintenance-sheet

@buresdv
Copy link
Owner

buresdv commented Mar 17, 2024

Since nobody reported any issues, I merged the fix into main (54c9ee2) and will consider this bug fixed by it

@buresdv buresdv closed this as completed Mar 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants