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

Fixes the multiline text heights #1427

Merged
merged 7 commits into from
Aug 4, 2023
Merged

Fixes the multiline text heights #1427

merged 7 commits into from
Aug 4, 2023

Conversation

diegoreymendez
Copy link
Contributor

@diegoreymendez diegoreymendez commented Aug 3, 2023 •
edited
Loading

Task/Issue URL: https://app.asana.com/0/1203137811378537/1205203672933054/f

Description:

Fixes the dynamic height calculation for multiline textfields in the Network Protection popover.

Steps to test this PR:

Test 1: Test the yellow alert box

  1. Go to

    macos-browser/LocalPackages/NetworkProtectionUI/Sources/NetworkProtectionUI/NetworkProtectionStatusView.swift

    Line 194 in 7a14c10

    Text(message)
  2. Replace variable message with a really long text (to force it to be multiline).
  3. Run the App
  4. Debug Menu > Network Protection > Simulate Failure > Simulate Tunnel Failure
  5. Connect NetP, your long message should show up in the yellow box.
  6. Make sure the popover height looks right and the text is multiline as expected.

Test 2: Test the feature status description

  1. Go to

    macos-browser/LocalPackages/NetworkProtectionUI/Sources/NetworkProtectionUI/NetworkProtectionStatusView.swift

    Line 223 in 7a14c10

    Text(UserText.networkProtectionStatusViewFeatureDesc)
  2. Replace model.featureStatusDescription with long text
  3. Run the app and make sure that the NetP popover height looks right with the long feature description
Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

diegoreymendez
diegoreymendez commented Aug 3, 2023
View reviewed changes
LocalPackages/NetworkProtectionUI/Sources/NetworkProtectionUI/NetworkProtectionPopover.swift
@@ -67,14 +67,11 @@ public final class NetworkProtectionPopover: NSPopover {
statusReporter: statusReporter,
menuItems: menuItems)

let controller: NSViewController
Copy link
Contributor Author

@diegoreymendez diegoreymendez Aug 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

There was no need to separate the declaration of controller in this line and then assign it in line 76, so I unified both.

diegoreymendez
diegoreymendez commented Aug 3, 2023
View reviewed changes
LocalPackages/NetworkProtectionUI/Sources/NetworkProtectionUI/NetworkProtectionPopover.swift
let view = NetworkProtectionStatusView(model: model).environment(\.dismiss, { [weak self] in
self?.close()
}).fixedSize()
.padding(.vertical, 10)
Copy link
Contributor Author

@diegoreymendez diegoreymendez Aug 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This was removed thanks to the multiline-text height fixes, which were causing the popover height to be off.

diegoreymendez
diegoreymendez commented Aug 3, 2023
View reviewed changes
LocalPackages/NetworkProtectionUI/Sources/NetworkProtectionUI/NetworkProtectionStatusView.swift
.multilineTextAlignment(.center)
.applyDescriptionAttributes(colorScheme: colorScheme)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure it was necessary to move this below, but I wanted the first thing to happen to be the fixing of the height.

diegoreymendez
diegoreymendez commented Aug 3, 2023
View reviewed changes
...kages/NetworkProtectionUI/Sources/NetworkProtectionUI/SwiftUI/MultilineTextHeightFixer.swift
/// Fixes the height for multiline text fields, which seem to suffer from a layout issue where
/// their height isn't properly honored.
///
private struct MultilineTextHeightFixer: ViewModifier {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We implemented this with @brindy 's help. We discussed it's probably reusable but that it may be best to leave it here for now.

@diegoreymendez diegoreymendez self-assigned this Aug 3, 2023
@diegoreymendez
Copy link
Contributor Author

@graeme - I'm assigning you as primary.

@brindy - Just ccing you, and assigning you as secondary in case there's anything you want to add or review (especially with the naming I chose for the dynamic height fixes).

@diegoreymendez diegoreymendez marked this pull request as ready for review August 3, 2023 15:07
@mallexxx mallexxx assigned mallexxx and unassigned diegoreymendez Aug 4, 2023
@mallexxx mallexxx self-requested a review August 4, 2023 06:46
mallexxx
mallexxx requested changes Aug 4, 2023
View reviewed changes
Copy link
Collaborator

@mallexxx mallexxx left a comment

Choose a reason for hiding this comment

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

Unfortunately it didn‘t work:

  • Edit StartError.simulateControllerFailureError to include long text
  • Choose simulate Controller error
  • Connect -> error appears 👌
  • Choose simulate Tunnel failure
  • Connect -> the text is messed up:
    Screenshot 2023-08-04 at 13 04 22

@mallexxx
Copy link
Collaborator

mallexxx commented Aug 4, 2023

maybe use AppKit? 😉

@diegoreymendez
Copy link
Contributor Author

maybe use AppKit? 😉

Heh, if only that was a quick solution I would... want to see first if I can resolve this with something immediate - I believe I know what the issue is.

@diegoreymendez
Copy link
Contributor Author

I wasn't able to reproduce the issue you're seeing, but I'm sure it's because onAppear is not executing it's code more than once in some circumstances.

Would you give this another go with my latest changes?

@diegoreymendez
Copy link
Contributor Author

@mallexxx - Just pushed the changes you suggested and they work fine for me too.

Thanks!

mallexxx
mallexxx approved these changes Aug 4, 2023
View reviewed changes
Copy link
Collaborator

@mallexxx mallexxx left a comment

Choose a reason for hiding this comment

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

LGTM

@diegoreymendez diegoreymendez merged commit 51ce762 into develop Aug 4, 2023
7 checks passed
@diegoreymendez diegoreymendez deleted the diego/fix-multiline-text-height branch August 4, 2023 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mallexxx mallexxx mallexxx approved these changes

@graeme graeme Awaiting requested review from graeme

@brindy brindy Awaiting requested review from brindy

Assignees

@mallexxx mallexxx

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@diegoreymendez @mallexxx