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

Fix a truncated session title in Japanese #33

Merged
merged 2 commits into from
Mar 15, 2024

Conversation

shimastripe
Copy link
Contributor

Result

Current This PR
Screenshot 2024-03-14 at 21 53 20 Screenshot 2024-03-14 at 22 14 18

WHY

  • I found a truncated title in Japanese.
    • I checked this in real device (iPhone 13 mini) too.

WHAT

@zunda-pixel
Copy link
Contributor

I think that using lineLimit(nil) is better.
https://developer.apple.com/documentation/swiftui/view/linelimit(_:)-513mb

@shimastripe
Copy link
Contributor Author

Thanks for your comment. Yes, I checked lineLimit(nil) at first. However it doesn't work.
I can't find this truncated text except for Japanese texts. (Perhaps it's a problem with the Japanese in SwiftUI Text?)

@zunda-pixel
Copy link
Contributor

zunda-pixel commented Mar 14, 2024

I may found problem. HStack line 79 in ScrollView is not needed. and also Spacer(line 80, 108), VStack(line 81).

I checked behavior on iPhone SE.

Suggestion Code

public var body: some View {
  ScrollView {
    VStack(alignment: .leading, spacing: 16) {
      Text(LocalizedStringKey(store.title), bundle: .module)
        .font(.title.bold())
      Text(LocalizedStringKey(store.description), bundle: .module)
        .font(.callout)
      if let requirements = store.requirements {
        VStack(alignment: .leading) {
          Text("Requirements", bundle: .module)
            .font(.subheadline.bold())
            .foregroundStyle(Color.accentColor)
          Text(LocalizedStringKey(requirements), bundle: .module)
            .font(.callout)
        }
        .padding()
        .overlay {
          RoundedRectangle(cornerRadius: 16)
            .stroke(Color.accentColor, lineWidth: 1)
        }
      }
      speakers
    }
    .padding(.horizontal)
    .padding(.bottom)
    .frame(maxWidth: 700)  // Readable content width for iPad
  }
  .sheet(item: $store.scope(state: \.destination?.safari, action: \.destination.safari)) { sheetStore in
    SafariViewRepresentation(url: sheetStore.url)
      .ignoresSafeArea()
  }
}

@shimastripe
Copy link
Contributor Author

shimastripe commented Mar 14, 2024

This PR Your Suggestion
Screenshot 2024-03-14 at 22 14 18 Screenshot 2024-03-15 at 1 16 26

Sorry, your suggestion code changes the paddings and the widths between components. I can't determine if those changes should be applied.
This PR is to patch where the specific session title is truncated in the current layout.

@d-date
Copy link
Member

d-date commented Mar 15, 2024

Hi @shimastripe @zunda-pixel ,

Thank you for making PR and great discussion.
I think @zunda-pixel 's suggestion is better since I know speaker bio is also truncated.
I forget why I put HStack (maybe for iPad, but works well without HStack...)

@shimastripe
Can you modify code as @zunda-pixel 's suggestion?

@shimastripe
Copy link
Contributor Author

@d-date Of course. I'll fix it. Is the horizontal margin of the speaker section okay as suggested? It can also be done as shown on the right.

suggestion fix speaker section horizontal padding
Screenshot 2024-03-15 at 1 16 26 Screenshot 2024-03-15 at 12 52 03

@d-date
Copy link
Member

d-date commented Mar 15, 2024

@shimastripe I prefer right one. Thank you!

@shimastripe
Copy link
Contributor Author

@d-date Thank you! I pushed the code in the right state. ( Thanks also @zunda-pixel )

@d-date d-date merged commit 726a02b into tryswift:main Mar 15, 2024
@d-date d-date mentioned this pull request Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants